RFC: include value in XML output for value-parameterized tests

Please consider this change which improves gtest's XML output to include the value for value-parameterized tests.

Value-parameterized tests have somewhat opaque names like:

  InstantiationName/FooTest.DoesBlah/0 for "meeny"
  InstantiationName/FooTest.DoesBlah/1 for "miny"
  InstantiationName/FooTest.DoesBlah/2 for "moe"

The existing pretty result printer prints the value, using operator<<, for any failed test like:

[  FAILED  ] InstantiationName/FooTest.DoesBlah/0, where GetParam() = meeny

This proposal prints the same information in the XML output. The value is printed as an attribute "param" of the testcase like:

<testcase
  name="DoesBlah/0"
  status="run"
  time="x.xxx"
  classname="InstantiationName/FooTest"
  param="GetParam() = meeny" />

There's another TestInfo member test_case_comment(). It's unclear if that member ever contains useful information. It's also unclear if that would be associated with the <testsuite /> or something else.

[wan, 2010-12-28 19:13:22]

Hi Joey,

Thanks for contributing.  Do you know if this works with popular
continuous build systems like Hudson?

I'm discussing this with our internal teams to make sure it doesn't
break existing tools that consume the XML.

On Mon, Dec 27, 2010 at 12:49 PM,  <joeyoravec@gmail.com> wrote:
> Reviewers: ,
>
> Description:
> Please consider this change which improves gtest's XML output to include
> the value for value-parameterized tests.
>
> Value-parameterized tests have somewhat opaque names like:
>
>  InstantiationName/FooTest.DoesBlah/0 for "meeny"
>  InstantiationName/FooTest.DoesBlah/1 for "miny"
>  InstantiationName/FooTest.DoesBlah/2 for "moe"
>
> The existing pretty result printer prints the value, using operator<<,
> for any failed test like:
>
> [  FAILED  ] InstantiationName/FooTest.DoesBlah/0, where GetParam() =
> meeny
>
> This proposal prints the same information in the XML output. The value
> is printed as an attribute "param" of the testcase like:
>
> <testcase
>  name="DoesBlah/0"
>  status="run"
>  time="x.xxx"
>  classname="InstantiationName/FooTest"
>  param="GetParam() = meeny" />

Hmm, we probably want to get rid of the "GetParam() = " prefix from the value.

>
> There's another TestInfo member test_case_comment(). It's unclear if
> that member ever contains useful information. It's also unclear if that
> would be associated with the <testsuite /> or something else.

This is used for type-parameterized tests and typed tests.

> Please review this at http://codereview.appspot.com/3773042/
>
> Affected files:
>  gtest.cc
>
>
> Index: gtest.cc
> ===================================================================
> --- gtest.cc    (revision 528)
> +++ gtest.cc    (working copy)
> @@ -3210,6 +3210,23 @@
>           << "\" classname=\"" << EscapeXmlAttribute(test_case_name).c_str()
>           << "\"" << TestPropertiesAsXmlAttributes(result).c_str();
>
> +  // Value-parameterized tests have opaque names like
> +  //  InstantiationName/FooTest.DoesBlah/0 for "meeny"
> +  //  InstantiationName/FooTest.DoesBlah/1 for "miny"
> +  //  InstantiationName/FooTest.DoesBlah/2 for "moe"
> +  //
> +  // Print the value. This will look like
> +  //  GetParam() = "meeny"
> +  // The string is generated by operator<< for the parameter's type
> +  //
> +  if (test_info.comment()[0] == '\0') {
> +    // no comment, do nothing
> +  } else {
> +    *stream << " param=\""
> +            << EscapeXmlAttribute(test_info.comment()).c_str()
> +            << "\"";
> +  }
> +
>   int failures = 0;
>   for (int i = 0; i < result.total_part_count(); ++i) {
>     const TestPartResult& part = result.GetTestPartResult(i);
>
>
>



-- 
Zhanyong

[joeyoravec, 2010-12-28 20:47:25]

No idea since I've never used Hudson. I wouldn't expect trouble since gtest
already has ReportProperty() which writes attributes in the same place. As I
got deeper into this I noticed:

- the XML doesn't describe the relation among value-parameterized tests.
Without hierarchy you have to parse strings to understand which tests are
related (like FooTest/0 .. FooTest/N).

- the <failure> tag returns the block of text, but could return finer
grained details like filename, line number, and the type (like
kNonFatalFailure)

...and there's probably more. An alternate proposal is that we maintain an
xml-junitreport logger that writes the format strictly, plus an xml-gtest
logger that is flexible to log every detail that gtest collects. This seems
manageable now that there's an event listener API.

-joey


2010/12/28 Zhanyong Wan (λx.x x) <wan@google.com>

> Hi Joey,
>
> Thanks for contributing.  Do you know if this works with popular
> continuous build systems like Hudson?
>
> I'm discussing this with our internal teams to make sure it doesn't
> break existing tools that consume the XML.
>
> On Mon, Dec 27, 2010 at 12:49 PM,  <joeyoravec@gmail.com> wrote:
> > Reviewers: ,
> >
> > Description:
> > Please consider this change which improves gtest's XML output to include
> > the value for value-parameterized tests.
> >
> > Value-parameterized tests have somewhat opaque names like:
> >
> >  InstantiationName/FooTest.DoesBlah/0 for "meeny"
> >  InstantiationName/FooTest.DoesBlah/1 for "miny"
> >  InstantiationName/FooTest.DoesBlah/2 for "moe"
> >
> > The existing pretty result printer prints the value, using operator<<,
> > for any failed test like:
> >
> > [  FAILED  ] InstantiationName/FooTest.DoesBlah/0, where GetParam() =
> > meeny
> >
> > This proposal prints the same information in the XML output. The value
> > is printed as an attribute "param" of the testcase like:
> >
> > <testcase
> >  name="DoesBlah/0"
> >  status="run"
> >  time="x.xxx"
> >  classname="InstantiationName/FooTest"
> >  param="GetParam() = meeny" />
>
> Hmm, we probably want to get rid of the "GetParam() = " prefix from the
> value.
>
> >
> > There's another TestInfo member test_case_comment(). It's unclear if
> > that member ever contains useful information. It's also unclear if that
> > would be associated with the <testsuite /> or something else.
>
> This is used for type-parameterized tests and typed tests.
>
> > Please review this at http://codereview.appspot.com/3773042/
> >
> > Affected files:
> >  gtest.cc
> >
> >
> > Index: gtest.cc
> > ===================================================================
> > --- gtest.cc    (revision 528)
> > +++ gtest.cc    (working copy)
> > @@ -3210,6 +3210,23 @@
> >           << "\" classname=\"" <<
> EscapeXmlAttribute(test_case_name).c_str()
> >           << "\"" << TestPropertiesAsXmlAttributes(result).c_str();
> >
> > +  // Value-parameterized tests have opaque names like
> > +  //  InstantiationName/FooTest.DoesBlah/0 for "meeny"
> > +  //  InstantiationName/FooTest.DoesBlah/1 for "miny"
> > +  //  InstantiationName/FooTest.DoesBlah/2 for "moe"
> > +  //
> > +  // Print the value. This will look like
> > +  //  GetParam() = "meeny"
> > +  // The string is generated by operator<< for the parameter's type
> > +  //
> > +  if (test_info.comment()[0] == '\0') {
> > +    // no comment, do nothing
> > +  } else {
> > +    *stream << " param=\""
> > +            << EscapeXmlAttribute(test_info.comment()).c_str()
> > +            << "\"";
> > +  }
> > +
> >   int failures = 0;
> >   for (int i = 0; i < result.total_part_count(); ++i) {
> >     const TestPartResult& part = result.GetTestPartResult(i);
> >
> >
> >
>
>
>
> --
> Zhanyong
>

[wan, 2010-12-28 21:47:48]

2010/12/28 Joey Oravec <joeyoravec@gmail.com>:
> No idea since I've never used Hudson. I wouldn't expect trouble since gtest
> already has ReportProperty() which writes attributes in the same place. As I

Actually I have no idea whether ReportProperty() works with Hudson. ;)

> got deeper into this I noticed:
>
> - the XML doesn't describe the relation among value-parameterized tests.
> Without hierarchy you have to parse strings to understand which tests are
> related (like FooTest/0 .. FooTest/N).

But these tests will be nested in the same parent testcase element, no?

> - the <failure> tag returns the block of text, but could return finer
> grained details like filename, line number, and the type (like
> kNonFatalFailure)
>
> ...and there's probably more. An alternate proposal is that we maintain an
> xml-junitreport logger that writes the format strictly, plus an xml-gtest
> logger that is flexible to log every detail that gtest collects. This seems
> manageable now that there's an event listener API.

I'd prefer not to create another XML format if we could avoid it.  If
you really care about this, I'd suggest to lobby the jUnit people to
extend the format such that we don't diverge.

>
> -joey
>
> 2010/12/28 Zhanyong Wan (λx.x x) <wan@google.com>
>>
>> Hi Joey,
>>
>> Thanks for contributing.  Do you know if this works with popular
>> continuous build systems like Hudson?
>>
>> I'm discussing this with our internal teams to make sure it doesn't
>> break existing tools that consume the XML.
>>
>> On Mon, Dec 27, 2010 at 12:49 PM,  <joeyoravec@gmail.com> wrote:
>> > Reviewers: ,
>> >
>> > Description:
>> > Please consider this change which improves gtest's XML output to include
>> > the value for value-parameterized tests.
>> >
>> > Value-parameterized tests have somewhat opaque names like:
>> >
>> >  InstantiationName/FooTest.DoesBlah/0 for "meeny"
>> >  InstantiationName/FooTest.DoesBlah/1 for "miny"
>> >  InstantiationName/FooTest.DoesBlah/2 for "moe"
>> >
>> > The existing pretty result printer prints the value, using operator<<,
>> > for any failed test like:
>> >
>> > [  FAILED  ] InstantiationName/FooTest.DoesBlah/0, where GetParam() =
>> > meeny
>> >
>> > This proposal prints the same information in the XML output. The value
>> > is printed as an attribute "param" of the testcase like:
>> >
>> > <testcase
>> >  name="DoesBlah/0"
>> >  status="run"
>> >  time="x.xxx"
>> >  classname="InstantiationName/FooTest"
>> >  param="GetParam() = meeny" />
>>
>> Hmm, we probably want to get rid of the "GetParam() = " prefix from the
>> value.
>>
>> >
>> > There's another TestInfo member test_case_comment(). It's unclear if
>> > that member ever contains useful information. It's also unclear if that
>> > would be associated with the <testsuite /> or something else.
>>
>> This is used for type-parameterized tests and typed tests.
>>
>> > Please review this at http://codereview.appspot.com/3773042/
>> >
>> > Affected files:
>> >  gtest.cc
>> >
>> >
>> > Index: gtest.cc
>> > ===================================================================
>> > --- gtest.cc    (revision 528)
>> > +++ gtest.cc    (working copy)
>> > @@ -3210,6 +3210,23 @@
>> >           << "\" classname=\"" <<
>> > EscapeXmlAttribute(test_case_name).c_str()
>> >           << "\"" << TestPropertiesAsXmlAttributes(result).c_str();
>> >
>> > +  // Value-parameterized tests have opaque names like
>> > +  //  InstantiationName/FooTest.DoesBlah/0 for "meeny"
>> > +  //  InstantiationName/FooTest.DoesBlah/1 for "miny"
>> > +  //  InstantiationName/FooTest.DoesBlah/2 for "moe"
>> > +  //
>> > +  // Print the value. This will look like
>> > +  //  GetParam() = "meeny"
>> > +  // The string is generated by operator<< for the parameter's type
>> > +  //
>> > +  if (test_info.comment()[0] == '\0') {
>> > +    // no comment, do nothing
>> > +  } else {
>> > +    *stream << " param=\""
>> > +            << EscapeXmlAttribute(test_info.comment()).c_str()
>> > +            << "\"";
>> > +  }
>> > +
>> >   int failures = 0;
>> >   for (int i = 0; i < result.total_part_count(); ++i) {
>> >     const TestPartResult& part = result.GetTestPartResult(i);
>> >
>> >
>> >
>>
>>
>>
>> --
>> Zhanyong
>
>



-- 
Zhanyong

[wan, 2011-01-10 19:14:05]

I would like to distinguish between value- and type- parameterized
tests.  For the former, I want an attribute like

  value_param=""meeny""

(note the '"' and the lack of "GetParam() = ").

For the latter, I want the attribute to look like:

  type_param="int"

Would you like to implement that?  Thanks,

2010/12/28 Zhanyong Wan (λx.x x) <wan@google.com>:
> 2010/12/28 Joey Oravec <joeyoravec@gmail.com>:
>> No idea since I've never used Hudson. I wouldn't expect trouble since gtest
>> already has ReportProperty() which writes attributes in the same place. As I
>
> Actually I have no idea whether ReportProperty() works with Hudson. ;)
>
>> got deeper into this I noticed:
>>
>> - the XML doesn't describe the relation among value-parameterized tests.
>> Without hierarchy you have to parse strings to understand which tests are
>> related (like FooTest/0 .. FooTest/N).
>
> But these tests will be nested in the same parent testcase element, no?
>
>> - the <failure> tag returns the block of text, but could return finer
>> grained details like filename, line number, and the type (like
>> kNonFatalFailure)
>>
>> ...and there's probably more. An alternate proposal is that we maintain an
>> xml-junitreport logger that writes the format strictly, plus an xml-gtest
>> logger that is flexible to log every detail that gtest collects. This seems
>> manageable now that there's an event listener API.
>
> I'd prefer not to create another XML format if we could avoid it.  If
> you really care about this, I'd suggest to lobby the jUnit people to
> extend the format such that we don't diverge.
>
>>
>> -joey
>>
>> 2010/12/28 Zhanyong Wan (λx.x x) <wan@google.com>
>>>
>>> Hi Joey,
>>>
>>> Thanks for contributing.  Do you know if this works with popular
>>> continuous build systems like Hudson?
>>>
>>> I'm discussing this with our internal teams to make sure it doesn't
>>> break existing tools that consume the XML.
>>>
>>> On Mon, Dec 27, 2010 at 12:49 PM,  <joeyoravec@gmail.com> wrote:
>>> > Reviewers: ,
>>> >
>>> > Description:
>>> > Please consider this change which improves gtest's XML output to include
>>> > the value for value-parameterized tests.
>>> >
>>> > Value-parameterized tests have somewhat opaque names like:
>>> >
>>> >  InstantiationName/FooTest.DoesBlah/0 for "meeny"
>>> >  InstantiationName/FooTest.DoesBlah/1 for "miny"
>>> >  InstantiationName/FooTest.DoesBlah/2 for "moe"
>>> >
>>> > The existing pretty result printer prints the value, using operator<<,
>>> > for any failed test like:
>>> >
>>> > [  FAILED  ] InstantiationName/FooTest.DoesBlah/0, where GetParam() =
>>> > meeny
>>> >
>>> > This proposal prints the same information in the XML output. The value
>>> > is printed as an attribute "param" of the testcase like:
>>> >
>>> > <testcase
>>> >  name="DoesBlah/0"
>>> >  status="run"
>>> >  time="x.xxx"
>>> >  classname="InstantiationName/FooTest"
>>> >  param="GetParam() = meeny" />
>>>
>>> Hmm, we probably want to get rid of the "GetParam() = " prefix from the
>>> value.
>>>
>>> >
>>> > There's another TestInfo member test_case_comment(). It's unclear if
>>> > that member ever contains useful information. It's also unclear if that
>>> > would be associated with the <testsuite /> or something else.
>>>
>>> This is used for type-parameterized tests and typed tests.
>>>
>>> > Please review this at http://codereview.appspot.com/3773042/
>>> >
>>> > Affected files:
>>> >  gtest.cc
>>> >
>>> >
>>> > Index: gtest.cc
>>> > ===================================================================
>>> > --- gtest.cc    (revision 528)
>>> > +++ gtest.cc    (working copy)
>>> > @@ -3210,6 +3210,23 @@
>>> >           << "\" classname=\"" <<
>>> > EscapeXmlAttribute(test_case_name).c_str()
>>> >           << "\"" << TestPropertiesAsXmlAttributes(result).c_str();
>>> >
>>> > +  // Value-parameterized tests have opaque names like
>>> > +  //  InstantiationName/FooTest.DoesBlah/0 for "meeny"
>>> > +  //  InstantiationName/FooTest.DoesBlah/1 for "miny"
>>> > +  //  InstantiationName/FooTest.DoesBlah/2 for "moe"
>>> > +  //
>>> > +  // Print the value. This will look like
>>> > +  //  GetParam() = "meeny"
>>> > +  // The string is generated by operator<< for the parameter's type
>>> > +  //
>>> > +  if (test_info.comment()[0] == '\0') {
>>> > +    // no comment, do nothing
>>> > +  } else {
>>> > +    *stream << " param=\""
>>> > +            << EscapeXmlAttribute(test_info.comment()).c_str()
>>> > +            << "\"";
>>> > +  }
>>> > +
>>> >   int failures = 0;
>>> >   for (int i = 0; i < result.total_part_count(); ++i) {
>>> >     const TestPartResult& part = result.GetTestPartResult(i);
>>> >
>>> >
>>> >
>>>
>>>
>>>
>>> --
>>> Zhanyong
>>
>>
>
>
>
> --
> Zhanyong
>



-- 
Zhanyong

[wan, 2011-01-10 19:16:22]

2011/1/10 Zhanyong Wan (λx.x x) <wan@google.com>:
> I would like to distinguish between value- and type- parameterized
> tests.  For the former, I want an attribute like
>
>  value_param="&quot;meeny&quot;"
>
> (note the '&quot;' and the lack of "GetParam() = ").
>
> For the latter, I want the attribute to look like:
>
>  type_param="int"
>
> Would you like to implement that?  Thanks,
>
> 2010/12/28 Zhanyong Wan (λx.x x) <wan@google.com>:
>> 2010/12/28 Joey Oravec <joeyoravec@gmail.com>:
>>> No idea since I've never used Hudson. I wouldn't expect trouble since gtest
>>> already has ReportProperty() which writes attributes in the same place. As I
>>
>> Actually I have no idea whether ReportProperty() works with Hudson. ;)
>>
>>> got deeper into this I noticed:
>>>
>>> - the XML doesn't describe the relation among value-parameterized tests.
>>> Without hierarchy you have to parse strings to understand which tests are
>>> related (like FooTest/0 .. FooTest/N).
>>
>> But these tests will be nested in the same parent testcase element, no?
>>
>>> - the <failure> tag returns the block of text, but could return finer
>>> grained details like filename, line number, and the type (like
>>> kNonFatalFailure)
>>>
>>> ...and there's probably more. An alternate proposal is that we maintain an
>>> xml-junitreport logger that writes the format strictly, plus an xml-gtest
>>> logger that is flexible to log every detail that gtest collects. This seems
>>> manageable now that there's an event listener API.
>>
>> I'd prefer not to create another XML format if we could avoid it.  If
>> you really care about this, I'd suggest to lobby the jUnit people to
>> extend the format such that we don't diverge.
>>
>>>
>>> -joey
>>>
>>> 2010/12/28 Zhanyong Wan (λx.x x) <wan@google.com>
>>>>
>>>> Hi Joey,
>>>>
>>>> Thanks for contributing.  Do you know if this works with popular
>>>> continuous build systems like Hudson?
>>>>
>>>> I'm discussing this with our internal teams to make sure it doesn't
>>>> break existing tools that consume the XML.
>>>>
>>>> On Mon, Dec 27, 2010 at 12:49 PM,  <joeyoravec@gmail.com> wrote:
>>>> > Reviewers: ,
>>>> >
>>>> > Description:
>>>> > Please consider this change which improves gtest's XML output to include
>>>> > the value for value-parameterized tests.
>>>> >
>>>> > Value-parameterized tests have somewhat opaque names like:
>>>> >
>>>> >  InstantiationName/FooTest.DoesBlah/0 for "meeny"
>>>> >  InstantiationName/FooTest.DoesBlah/1 for "miny"
>>>> >  InstantiationName/FooTest.DoesBlah/2 for "moe"
>>>> >
>>>> > The existing pretty result printer prints the value, using operator<<,
>>>> > for any failed test like:
>>>> >
>>>> > [  FAILED  ] InstantiationName/FooTest.DoesBlah/0, where GetParam() =
>>>> > meeny
>>>> >
>>>> > This proposal prints the same information in the XML output. The value
>>>> > is printed as an attribute "param" of the testcase like:
>>>> >
>>>> > <testcase
>>>> >  name="DoesBlah/0"
>>>> >  status="run"
>>>> >  time="x.xxx"
>>>> >  classname="InstantiationName/FooTest"
>>>> >  param="GetParam() = meeny" />
>>>>
>>>> Hmm, we probably want to get rid of the "GetParam() = " prefix from the
>>>> value.
>>>>
>>>> >
>>>> > There's another TestInfo member test_case_comment(). It's unclear if
>>>> > that member ever contains useful information. It's also unclear if that
>>>> > would be associated with the <testsuite /> or something else.
>>>>
>>>> This is used for type-parameterized tests and typed tests.
>>>>
>>>> > Please review this at http://codereview.appspot.com/3773042/
>>>> >
>>>> > Affected files:
>>>> >  gtest.cc
>>>> >
>>>> >
>>>> > Index: gtest.cc
>>>> > ===================================================================
>>>> > --- gtest.cc    (revision 528)
>>>> > +++ gtest.cc    (working copy)
>>>> > @@ -3210,6 +3210,23 @@
>>>> >           << "\" classname=\"" <<
>>>> > EscapeXmlAttribute(test_case_name).c_str()
>>>> >           << "\"" << TestPropertiesAsXmlAttributes(result).c_str();
>>>> >
>>>> > +  // Value-parameterized tests have opaque names like
>>>> > +  //  InstantiationName/FooTest.DoesBlah/0 for "meeny"
>>>> > +  //  InstantiationName/FooTest.DoesBlah/1 for "miny"
>>>> > +  //  InstantiationName/FooTest.DoesBlah/2 for "moe"
>>>> > +  //
>>>> > +  // Print the value. This will look like
>>>> > +  //  GetParam() = "meeny"
>>>> > +  // The string is generated by operator<< for the parameter's type
>>>> > +  //
>>>> > +  if (test_info.comment()[0] == '\0') {
>>>> > +    // no comment, do nothing
>>>> > +  } else {
>>>> > +    *stream << " param=\""
>>>> > +            << EscapeXmlAttribute(test_info.comment()).c_str()
>>>> > +            << "\"";
>>>> > +  }
>>>> > +
>>>> >   int failures = 0;
>>>> >   for (int i = 0; i < result.total_part_count(); ++i) {
>>>> >     const TestPartResult& part = result.GetTestPartResult(i);
>>>> >
>>>> >
>>>> >
>>>>
>>>>
>>>>
>>>> --
>>>> Zhanyong
>>>
>>>
>>
>>
>>
>> --
>> Zhanyong
>>
>
>
>
> --
> Zhanyong
>

This is now tracked by http://code.google.com/p/googletest/issues/detail?id=301

-- 
Zhanyong

[joeyoravec, 2011-01-11 16:02:22]

Sure, I agree it would be way more useful to print that way. I can work on
it, but I wonder if you could explain how to get from a TestInfo to the
corresponding Test*. In file gtest-param-util.h in
ParameterizedTestCaseInfo::RegisterTests() we see:

   std::string comment = "GetParam() = " + PrintToString(*param_it);

which generates the string, then calls MakeAndRegisterTestInfo which records
the comment string. Similarly in file gtest-internal.h in
TypeParameterizedTest::Register() we see:

   String::Format("TypeParam = %s", GetTypeName<Type>().c_str())

where it's also recording a comment string. Much later when gtest is
printing results, it's operating on a TestInfo object which contains these
strings. Of course I printed these in this first draft because they're
readily accessible.

Regarding your proposal, for a given TestInfo we'd want to know if it was
value-parameterized or type-parameterized and print the parameter. I was
assuming I could follow some pointer from the TestInfo to the Test* since
the (derived) Test object records the parameter as a member. But I only saw
the factory... Isn't there some chain of pointers that I could follow to get
from a TestInfo to the corresponding Test* object?

My alternative is to record the parameter information as a string in
TestInfo when MakeAndRegisterTestInfo is called. This would probably work,
but seems preferable for the function that's doing the printing (like
OutputXmlTestInfo or the pretty result printer) to access the member
directly so it can render the string exactly how it wants.

-joey

2011/1/10 Zhanyong Wan (λx.x x) <wan@google.com>

> I would like to distinguish between value- and type- parameterized
> tests.  For the former, I want an attribute like
>
>  value_param="&quot;meeny&quot;"
>
> (note the '&quot;' and the lack of "GetParam() = ").
>
> For the latter, I want the attribute to look like:
>
>  type_param="int"
>
> Would you like to implement that?  Thanks,
>
> 2010/12/28 Zhanyong Wan (λx.x x) <wan@google.com>:
>  > 2010/12/28 Joey Oravec <joeyoravec@gmail.com>:
> >> No idea since I've never used Hudson. I wouldn't expect trouble since
> gtest
> >> already has ReportProperty() which writes attributes in the same place.
> As I
> >
> > Actually I have no idea whether ReportProperty() works with Hudson. ;)
> >
> >> got deeper into this I noticed:
> >>
> >> - the XML doesn't describe the relation among value-parameterized tests.
> >> Without hierarchy you have to parse strings to understand which tests
> are
> >> related (like FooTest/0 .. FooTest/N).
> >
> > But these tests will be nested in the same parent testcase element, no?
> >
> >> - the <failure> tag returns the block of text, but could return finer
> >> grained details like filename, line number, and the type (like
> >> kNonFatalFailure)
> >>
> >> ...and there's probably more. An alternate proposal is that we maintain
> an
> >> xml-junitreport logger that writes the format strictly, plus an
> xml-gtest
> >> logger that is flexible to log every detail that gtest collects. This
> seems
> >> manageable now that there's an event listener API.
> >
> > I'd prefer not to create another XML format if we could avoid it.  If
> > you really care about this, I'd suggest to lobby the jUnit people to
> > extend the format such that we don't diverge.
> >
> >>
> >> -joey
> >>
> >> 2010/12/28 Zhanyong Wan (λx.x x) <wan@google.com>
> >>>
> >>> Hi Joey,
> >>>
> >>> Thanks for contributing.  Do you know if this works with popular
> >>> continuous build systems like Hudson?
> >>>
> >>> I'm discussing this with our internal teams to make sure it doesn't
> >>> break existing tools that consume the XML.
> >>>
> >>> On Mon, Dec 27, 2010 at 12:49 PM,  <joeyoravec@gmail.com> wrote:
> >>> > Reviewers: ,
> >>> >
> >>> > Description:
> >>> > Please consider this change which improves gtest's XML output to
> include
> >>> > the value for value-parameterized tests.
> >>> >
> >>> > Value-parameterized tests have somewhat opaque names like:
> >>> >
> >>> >  InstantiationName/FooTest.DoesBlah/0 for "meeny"
> >>> >  InstantiationName/FooTest.DoesBlah/1 for "miny"
> >>> >  InstantiationName/FooTest.DoesBlah/2 for "moe"
> >>> >
> >>> > The existing pretty result printer prints the value, using
> operator<<,
> >>> > for any failed test like:
> >>> >
> >>> > [  FAILED  ] InstantiationName/FooTest.DoesBlah/0, where GetParam() =
> >>> > meeny
> >>> >
> >>> > This proposal prints the same information in the XML output. The
> value
> >>> > is printed as an attribute "param" of the testcase like:
> >>> >
> >>> > <testcase
> >>> >  name="DoesBlah/0"
> >>> >  status="run"
> >>> >  time="x.xxx"
> >>> >  classname="InstantiationName/FooTest"
> >>> >  param="GetParam() = meeny" />
> >>>
> >>> Hmm, we probably want to get rid of the "GetParam() = " prefix from the
> >>> value.
> >>>
> >>> >
> >>> > There's another TestInfo member test_case_comment(). It's unclear if
> >>> > that member ever contains useful information. It's also unclear if
> that
> >>> > would be associated with the <testsuite /> or something else.
> >>>
> >>> This is used for type-parameterized tests and typed tests.
> >>>
> >>> > Please review this at http://codereview.appspot.com/3773042/
> >>> >
> >>> > Affected files:
> >>> >  gtest.cc
> >>> >
> >>> >
> >>> > Index: gtest.cc
> >>> > ===================================================================
> >>> > --- gtest.cc    (revision 528)
> >>> > +++ gtest.cc    (working copy)
> >>> > @@ -3210,6 +3210,23 @@
> >>> >           << "\" classname=\"" <<
> >>> > EscapeXmlAttribute(test_case_name).c_str()
> >>> >           << "\"" << TestPropertiesAsXmlAttributes(result).c_str();
> >>> >
> >>> > +  // Value-parameterized tests have opaque names like
> >>> > +  //  InstantiationName/FooTest.DoesBlah/0 for "meeny"
> >>> > +  //  InstantiationName/FooTest.DoesBlah/1 for "miny"
> >>> > +  //  InstantiationName/FooTest.DoesBlah/2 for "moe"
> >>> > +  //
> >>> > +  // Print the value. This will look like
> >>> > +  //  GetParam() = "meeny"
> >>> > +  // The string is generated by operator<< for the parameter's type
> >>> > +  //
> >>> > +  if (test_info.comment()[0] == '\0') {
> >>> > +    // no comment, do nothing
> >>> > +  } else {
> >>> > +    *stream << " param=\""
> >>> > +            << EscapeXmlAttribute(test_info.comment()).c_str()
> >>> > +            << "\"";
> >>> > +  }
> >>> > +
> >>> >   int failures = 0;
> >>> >   for (int i = 0; i < result.total_part_count(); ++i) {
> >>> >     const TestPartResult& part = result.GetTestPartResult(i);
> >>> >
> >>> >
> >>> >
> >>>
> >>>
> >>>
> >>> --
> >>> Zhanyong
> >>
> >>
> >
> >
> >
> > --
> > Zhanyong
> >
>
>
>
> --
> Zhanyong
>

[wan, 2011-01-13 08:39:50]

2011/1/11 Joey Oravec <joeyoravec@gmail.com>:
> Sure, I agree it would be way more useful to print that way. I can work on
> it, but I wonder if you could explain how to get from a TestInfo to the
> corresponding Test*. In file gtest-param-util.h in
> ParameterizedTestCaseInfo::RegisterTests() we see:
>
>    std::string comment = "GetParam() = " + PrintToString(*param_it);
>
> which generates the string, then calls MakeAndRegisterTestInfo which records
> the comment string. Similarly in file gtest-internal.h in
> TypeParameterizedTest::Register() we see:
>
>    String::Format("TypeParam = %s", GetTypeName<Type>().c_str())
>
> where it's also recording a comment string. Much later when gtest is
> printing results, it's operating on a TestInfo object which contains these
> strings. Of course I printed these in this first draft because they're
> readily accessible.
>
> Regarding your proposal, for a given TestInfo we'd want to know if it was
> value-parameterized or type-parameterized and print the parameter.

Yes.

> I was
> assuming I could follow some pointer from the TestInfo to the Test* since
> the (derived) Test object records the parameter as a member. But I only saw
> the factory... Isn't there some chain of pointers that I could follow to get
> from a TestInfo to the corresponding Test* object?

No.  To isolate the tests, gtest only creates a Test object after the
previous test method has finished and the previous Test object has
been deleted.  At any time, there is at most one Test object alive.
Since the XML report is generated at the end of the test run, all Test
objects have been deleted by that time, so you cannot get a pointer to
any of them, regardless of where you look.

> My alternative is to record the parameter information as a string in
> TestInfo when MakeAndRegisterTestInfo is called. This would probably work,

Sounds good.

> but seems preferable for the function that's doing the printing (like
> OutputXmlTestInfo or the pretty result printer) to access the member
> directly so it can render the string exactly how it wants.

Even if the printer function has access to the Test object, how can it
extract the value parameter or type parameter from the object?  The
only way to get that information is to ask the Test object to provide
it in an untyped format (i.e. the Test object must do the rendering),
as types are not first-class values in C++.  So I don't think it's
possible to do what you want.

> -joey
>
> 2011/1/10 Zhanyong Wan (λx.x x) <wan@google.com>
>>
>> I would like to distinguish between value- and type- parameterized
>> tests.  For the former, I want an attribute like
>>
>>  value_param="&quot;meeny&quot;"
>>
>> (note the '&quot;' and the lack of "GetParam() = ").
>>
>> For the latter, I want the attribute to look like:
>>
>>  type_param="int"
>>
>> Would you like to implement that?  Thanks,
>>
>> 2010/12/28 Zhanyong Wan (λx.x x) <wan@google.com>:
>> > 2010/12/28 Joey Oravec <joeyoravec@gmail.com>:
>> >> No idea since I've never used Hudson. I wouldn't expect trouble since
>> >> gtest
>> >> already has ReportProperty() which writes attributes in the same place.
>> >> As I
>> >
>> > Actually I have no idea whether ReportProperty() works with Hudson. ;)
>> >
>> >> got deeper into this I noticed:
>> >>
>> >> - the XML doesn't describe the relation among value-parameterized
>> >> tests.
>> >> Without hierarchy you have to parse strings to understand which tests
>> >> are
>> >> related (like FooTest/0 .. FooTest/N).
>> >
>> > But these tests will be nested in the same parent testcase element, no?
>> >
>> >> - the <failure> tag returns the block of text, but could return finer
>> >> grained details like filename, line number, and the type (like
>> >> kNonFatalFailure)
>> >>
>> >> ...and there's probably more. An alternate proposal is that we maintain
>> >> an
>> >> xml-junitreport logger that writes the format strictly, plus an
>> >> xml-gtest
>> >> logger that is flexible to log every detail that gtest collects. This
>> >> seems
>> >> manageable now that there's an event listener API.
>> >
>> > I'd prefer not to create another XML format if we could avoid it.  If
>> > you really care about this, I'd suggest to lobby the jUnit people to
>> > extend the format such that we don't diverge.
>> >
>> >>
>> >> -joey
>> >>
>> >> 2010/12/28 Zhanyong Wan (λx.x x) <wan@google.com>
>> >>>
>> >>> Hi Joey,
>> >>>
>> >>> Thanks for contributing.  Do you know if this works with popular
>> >>> continuous build systems like Hudson?
>> >>>
>> >>> I'm discussing this with our internal teams to make sure it doesn't
>> >>> break existing tools that consume the XML.
>> >>>
>> >>> On Mon, Dec 27, 2010 at 12:49 PM,  <joeyoravec@gmail.com> wrote:
>> >>> > Reviewers: ,
>> >>> >
>> >>> > Description:
>> >>> > Please consider this change which improves gtest's XML output to
>> >>> > include
>> >>> > the value for value-parameterized tests.
>> >>> >
>> >>> > Value-parameterized tests have somewhat opaque names like:
>> >>> >
>> >>> >  InstantiationName/FooTest.DoesBlah/0 for "meeny"
>> >>> >  InstantiationName/FooTest.DoesBlah/1 for "miny"
>> >>> >  InstantiationName/FooTest.DoesBlah/2 for "moe"
>> >>> >
>> >>> > The existing pretty result printer prints the value, using
>> >>> > operator<<,
>> >>> > for any failed test like:
>> >>> >
>> >>> > [  FAILED  ] InstantiationName/FooTest.DoesBlah/0, where GetParam()
>> >>> > =
>> >>> > meeny
>> >>> >
>> >>> > This proposal prints the same information in the XML output. The
>> >>> > value
>> >>> > is printed as an attribute "param" of the testcase like:
>> >>> >
>> >>> > <testcase
>> >>> >  name="DoesBlah/0"
>> >>> >  status="run"
>> >>> >  time="x.xxx"
>> >>> >  classname="InstantiationName/FooTest"
>> >>> >  param="GetParam() = meeny" />
>> >>>
>> >>> Hmm, we probably want to get rid of the "GetParam() = " prefix from
>> >>> the
>> >>> value.
>> >>>
>> >>> >
>> >>> > There's another TestInfo member test_case_comment(). It's unclear if
>> >>> > that member ever contains useful information. It's also unclear if
>> >>> > that
>> >>> > would be associated with the <testsuite /> or something else.
>> >>>
>> >>> This is used for type-parameterized tests and typed tests.
>> >>>
>> >>> > Please review this at http://codereview.appspot.com/3773042/
>> >>> >
>> >>> > Affected files:
>> >>> >  gtest.cc
>> >>> >
>> >>> >
>> >>> > Index: gtest.cc
>> >>> > ===================================================================
>> >>> > --- gtest.cc    (revision 528)
>> >>> > +++ gtest.cc    (working copy)
>> >>> > @@ -3210,6 +3210,23 @@
>> >>> >           << "\" classname=\"" <<
>> >>> > EscapeXmlAttribute(test_case_name).c_str()
>> >>> >           << "\"" << TestPropertiesAsXmlAttributes(result).c_str();
>> >>> >
>> >>> > +  // Value-parameterized tests have opaque names like
>> >>> > +  //  InstantiationName/FooTest.DoesBlah/0 for "meeny"
>> >>> > +  //  InstantiationName/FooTest.DoesBlah/1 for "miny"
>> >>> > +  //  InstantiationName/FooTest.DoesBlah/2 for "moe"
>> >>> > +  //
>> >>> > +  // Print the value. This will look like
>> >>> > +  //  GetParam() = "meeny"
>> >>> > +  // The string is generated by operator<< for the parameter's type
>> >>> > +  //
>> >>> > +  if (test_info.comment()[0] == '\0') {
>> >>> > +    // no comment, do nothing
>> >>> > +  } else {
>> >>> > +    *stream << " param=\""
>> >>> > +            << EscapeXmlAttribute(test_info.comment()).c_str()
>> >>> > +            << "\"";
>> >>> > +  }
>> >>> > +
>> >>> >   int failures = 0;
>> >>> >   for (int i = 0; i < result.total_part_count(); ++i) {
>> >>> >     const TestPartResult& part = result.GetTestPartResult(i);
>> >>> >
>> >>> >
>> >>> >
>> >>>
>> >>>
>> >>>
>> >>> --
>> >>> Zhanyong
>> >>
>> >>
>> >
>> >
>> >
>> > --
>> > Zhanyong
>> >
>>
>>
>>
>> --
>> Zhanyong
>
>



-- 
Zhanyong

[joeyoravec, 2011-01-18 15:56:23]

Modifies XML output to print value_param="" or type_param=""

[wan, 2011-01-19 01:16:42]

Nice!  Thanks for working on this.

I'm particularly busy this week, so it may be a while before I can get
back to you.  Perhaps Vlad can find time to comment before I get to
it?  Thanks,

2011/1/18 Joey Oravec <joeyoravec@gmail.com>:
> 2011/1/13 Zhanyong Wan (λx.x x) <wan@google.com>
>>
>> > My alternative is to record the parameter information as a string in
>> > TestInfo when MakeAndRegisterTestInfo is called. This would probably
>> > work,
>> Sounds good.
>
> Please review patch set 2 at http://codereview.appspot.com/3773042/. This
> diff was generated against SVN trunk. This patch adds members to TestInfo
> that track if the test has a parameter, and if so then a string
> representation of that parameter. It also modifies the XML unit test result
> printer to record:
>
> value_param="xxx"
> or
> type_param="yyy"
>
> as you requested. I've tested this on VS2010 using:
>
> sample1_unittest.cc: Implements a TEST() with no parameters
> sample6_unittest.cc: Implements a TYPED_TEST() which is type-parameterized
> sample7_unittest.cc: Implements a TEST_P() which is value-parameterized
>
> I've attached the test_detail.xml from those testcases. As expected gtest
> prints value_param, type_param, or nothing.
>
> -joey



-- 
Zhanyong

[vladl_google.com, 2011-01-19 16:36:08]

I have taken a look. Rietveld is for some reason unable to show side-by-side
diffs, so I will comment here. The patch seems OK, with a couple of notes:
 * I prefer to scrape the test and test case comments instead of adding more
parameters to the the test creation routines. Longer function parameter
lists and data member lists make code harder to understand and debug.
 * This change needs tests verifying it.

If Joey is fine with it, I can take the patch, make the changes and commit
it.

Also, a note not related to the patch itself. Currently, an instantiation of
a value-parameterized test case  template every instantiation produces just
one test case. For type-parameterized test cases, a template produces one
test case per parameter. This better reflects the structure of the original
template. I think we should change the grouping of value parameterized
tests, to make it more logical and consistent with type-parameterized tests.
But it's definitely not part of this patch, though.

- Vlad

2011/1/18 Zhanyong Wan (λx.x x) <wan@google.com>

> Nice!  Thanks for working on this.
>
> I'm particularly busy this week, so it may be a while before I can get
> back to you.  Perhaps Vlad can find time to comment before I get to
> it?  Thanks,
>
> 2011/1/18 Joey Oravec <joeyoravec@gmail.com>:
> > 2011/1/13 Zhanyong Wan (λx.x x) <wan@google.com>
> >>
> >> > My alternative is to record the parameter information as a string in
> >> > TestInfo when MakeAndRegisterTestInfo is called. This would probably
> >> > work,
> >> Sounds good.
> >
> > Please review patch set 2 at http://codereview.appspot.com/3773042/.
> This
> > diff was generated against SVN trunk. This patch adds members to TestInfo
> > that track if the test has a parameter, and if so then a string
> > representation of that parameter. It also modifies the XML unit test
> result
> > printer to record:
> >
> > value_param="xxx"
> > or
> > type_param="yyy"
> >
> > as you requested. I've tested this on VS2010 using:
> >
> > sample1_unittest.cc: Implements a TEST() with no parameters
> > sample6_unittest.cc: Implements a TYPED_TEST() which is
> type-parameterized
> > sample7_unittest.cc: Implements a TEST_P() which is value-parameterized
> >
> > I've attached the test_detail.xml from those testcases. As expected gtest
> > prints value_param, type_param, or nothing.
> >
> > -joey
>
>
>
> --
> Zhanyong
>

[joeyoravec, 2011-01-19 19:29:15]

2011/1/19 Vlad Losev <vladl@google.com>

> I have taken a look. Rietveld is for some reason unable to show
> side-by-side diffs, so I will comment here. The patch seems OK, with a
> couple of notes:
>  * I prefer to scrape the test and test case comments instead of adding
> more parameters to the the test creation routines. Longer function parameter
> lists and data member lists make code harder to understand and debug.
>

Did you mean to write scrap (get rid of) the test comment and test case
comment? Right now they're described in gtest-internal.h above
MakeAndRegisterTestInfo as:

test_case_comment: A comment on the test case that will be included in the
output
comment: A comment on the test that will be included in the output

Based on the description I would expect these members to hold some
human-readable description of the testcase and test, but as far as I can
tell they're completely internal and there's no way for a user to fill these
fields when writing a test. Right now they're used for:

type-parameterized tests: set the test_case_comment with "TypeParam = ..."
and leave comment unused
value-parameterized tests: set the comment with "GetParam = ..." and leave
test_case_comment unused

...and they're printed by the pretty result printer. Yes I think that's
redundant, and my patch probably didn't go far enough. I think:

1. We should replace any existing use of test_case_comment() and comment()
with the appropriate value_param() or type_param(). This is mainly going to
affect pretty result printer.
2. As the code stands we cannot avoid the has_X_param boolean because that
lets us distinguish between no param, and a param which printed as a
zero-length string.
3. Finally if test_case_comment() and comment() don't have any use, we could
get rid of them.

Let me know what you think. I am willing to do the legwork if/when we're in
agreement.

I'll send you a separate email on the other topic (value- versus type-
cleanup) off list since I didn't totally understand it.

-joey

[vladl_google.com, 2011-01-20 03:19:25]

2011/1/19 Joey Oravec <joeyoravec@gmail.com>

> 2011/1/19 Vlad Losev <vladl@google.com>
>
>  I have taken a look. Rietveld is for some reason unable to show
>> side-by-side diffs, so I will comment here. The patch seems OK, with a
>> couple of notes:
>>  * I prefer to scrape the test and test case comments instead of adding
>> more parameters to the the test creation routines. Longer function parameter
>> lists and data member lists make code harder to understand and debug.
>>
>
> Did you mean to write scrap (get rid of) the test comment and test case
> comment?
>

No, I mean scrape, as in parse and extract information from. Right now they
are used to carry parameter information for the stdout printer, but can also
be parsed by the XML printer. Because gtest controls information that ends
up there, parsing it won't present a problem.


> Right now they're described in gtest-internal.h above
> MakeAndRegisterTestInfo as:
>
> test_case_comment: A comment on the test case that will be included in the
> output
> comment: A comment on the test that will be included in the output
>
> Based on the description I would expect these members to hold some
> human-readable description of the testcase and test, but as far as I can
> tell they're completely internal and there's no way for a user to fill these
> fields when writing a test. Right now they're used for:
>
> type-parameterized tests: set the test_case_comment with "TypeParam = ..."
> and leave comment unused
> value-parameterized tests: set the comment with "GetParam = ..." and leave
> test_case_comment unused
>
> ...and they're printed by the pretty result printer. Yes I think that's
> redundant, and my patch probably didn't go far enough. I think:
>
> 1. We should replace any existing use of test_case_comment() and comment()
> with the appropriate value_param() or type_param(). This is mainly going to
> affect pretty result printer.
> 2. As the code stands we cannot avoid the has_X_param boolean because that
> lets us distinguish between no param, and a param which printed as a
> zero-length string.
> 3. Finally if test_case_comment() and comment() don't have any use, we
> could get rid of them.
>
> Let me know what you think. I am willing to do the legwork if/when we're in
> agreement.
>
> I'll send you a separate email on the other topic (value- versus type-
> cleanup) off list since I didn't totally understand it.
>

> -joey
>

[wan, 2011-01-20 05:59:12]

2011/1/19 Vlad Losev <vladl@google.com>:
>
>
> 2011/1/19 Joey Oravec <joeyoravec@gmail.com>
>>
>> 2011/1/19 Vlad Losev <vladl@google.com>
>>>
>>> I have taken a look. Rietveld is for some reason unable to show
>>> side-by-side diffs, so I will comment here. The patch seems OK, with a
>>> couple of notes:
>>>  * I prefer to scrape the test and test case comments instead of adding
>>> more parameters to the the test creation routines. Longer function parameter
>>> lists and data member lists make code harder to understand and debug.
>>
>>
>> Did you mean to write scrap (get rid of) the test comment and test case
>> comment?
>
> No, I mean scrape, as in parse and extract information from. Right now they
> are used to carry parameter information for the stdout printer, but can also
> be parsed by the XML printer. Because gtest controls information that ends
> up there, parsing it won't present a problem.

Please do not go that route.  The other direction is much more
principled and cleaner.

>> Right now they're described in gtest-internal.h above
>> MakeAndRegisterTestInfo as:
>>
>> test_case_comment: A comment on the test case that will be included in the
>> output
>> comment: A comment on the test that will be included in the output
>>
>> Based on the description I would expect these members to hold some
>> human-readable description of the testcase and test, but as far as I can
>> tell they're completely internal and there's no way for a user to fill these
>> fields when writing a test. Right now they're used for:
>>
>> type-parameterized tests: set the test_case_comment with "TypeParam = ..."
>> and leave comment unused
>> value-parameterized tests: set the comment with "GetParam = ..." and leave
>> test_case_comment unused
>>
>> ...and they're printed by the pretty result printer. Yes I think that's
>> redundant, and my patch probably didn't go far enough. I think:
>>
>> 1. We should replace any existing use of test_case_comment() and comment()
>> with the appropriate value_param() or type_param(). This is mainly going to
>> affect pretty result printer.
>> 2. As the code stands we cannot avoid the has_X_param boolean because that
>> lets us distinguish between no param, and a param which printed as a
>> zero-length string.
>> 3. Finally if test_case_comment() and comment() don't have any use, we
>> could get rid of them.

I completely agree with you here except #2.  When I asked you to store
the text presentation of the value- and type- parameters in TestInfo,
I actually meant to replace the *comment() methods -- if necessary, we
can always calculate the "comments" from value_param() and
type_param(), so the "comments" are just a derivative and can be
removed.

As to #2, since no value or type should be printed as the empty
string, we can safely use the empty string to indicate "no param".

If the value itself is the empty string, it shall be printed as two
double-quotes (i.e. "\"\"" when written as a C string literal), which
isn't the same as an empty string.

>>
>> Let me know what you think. I am willing to do the legwork if/when we're
>> in agreement.

Your plan sounds good!  I'll review it when you are ready.

>>
>> I'll send you a separate email on the other topic (value- versus type-
>> cleanup) off list since I didn't totally understand it.
>>
>>
>> -joey
>



-- 
Zhanyong

[wan, 2011-01-20 06:07:02]

2011/1/19 Vlad Losev <vladl@google.com>:
> Also, a note not related to the patch itself. Currently, an instantiation of
> a value-parameterized test case  template every instantiation produces just
> one test case. For type-parameterized test cases, a template produces one
> test case per parameter. This better reflects the structure of the original
> template. I think we should change the grouping of value parameterized
> tests, to make it more logical and consistent with type-parameterized tests.

The grouping of value-parameterized tests was a conscious decision by
us.  It reduces the number of test cases (and thus reduces the number
of SetUpTestCase() and TearDownTestCase() calls, which makes the test
program run faster).

Unfortunately we couldn't do the same with type-parameterized tests,
as the same test case cannot hold tests with different type
parameters.

There's some inconsistency here, but I think it's not a big problem.
Also, changing the design at this stage is almost guaranteed to break
some existing users.  I think we should leave things as they are.


> But it's definitely not part of this patch, though.
> - Vlad

-- 
Zhanyong

[vladl_google.com, 2011-01-20 19:43:42]

2011/1/20 Zhanyong Wan (λx.x x) <wan@google.com>

> 2011/1/19 Vlad Losev <vladl@google.com>:
> >
> >
> > 2011/1/19 Joey Oravec <joeyoravec@gmail.com>
> >>
> >> 2011/1/19 Vlad Losev <vladl@google.com>
> >>>
> >>> I have taken a look. Rietveld is for some reason unable to show
> >>> side-by-side diffs, so I will comment here. The patch seems OK, with a
> >>> couple of notes:
> >>>  * I prefer to scrape the test and test case comments instead of adding
> >>> more parameters to the the test creation routines. Longer function
> parameter
> >>> lists and data member lists make code harder to understand and debug.
> >>
> >>
> >> Did you mean to write scrap (get rid of) the test comment and test case
> >> comment?
> >
> > No, I mean scrape, as in parse and extract information from. Right now
> they
> > are used to carry parameter information for the stdout printer, but can
> also
> > be parsed by the XML printer. Because gtest controls information that
> ends
> > up there, parsing it won't present a problem.
>
> Please do not go that route.  The other direction is much more
> principled and cleaner.
>
> >> Right now they're described in gtest-internal.h above
> >> MakeAndRegisterTestInfo as:
> >>
> >> test_case_comment: A comment on the test case that will be included in
> the
> >> output
> >> comment: A comment on the test that will be included in the output
> >>
> >> Based on the description I would expect these members to hold some
> >> human-readable description of the testcase and test, but as far as I can
> >> tell they're completely internal and there's no way for a user to fill
> these
> >> fields when writing a test. Right now they're used for:
> >>
> >> type-parameterized tests: set the test_case_comment with "TypeParam =
> ..."
> >> and leave comment unused
> >> value-parameterized tests: set the comment with "GetParam = ..." and
> leave
> >> test_case_comment unused
> >>
> >> ...and they're printed by the pretty result printer. Yes I think that's
> >> redundant, and my patch probably didn't go far enough. I think:
> >>
> >> 1. We should replace any existing use of test_case_comment() and
> comment()
> >> with the appropriate value_param() or type_param(). This is mainly going
> to
> >> affect pretty result printer.
> >> 2. As the code stands we cannot avoid the has_X_param boolean because
> that
> >> lets us distinguish between no param, and a param which printed as a
> >> zero-length string.
> >> 3. Finally if test_case_comment() and comment() don't have any use, we
> >> could get rid of them.
>
> I completely agree with you here except #2.  When I asked you to store
> the text presentation of the value- and type- parameters in TestInfo,
> I actually meant to replace the *comment() methods -- if necessary, we
> can always calculate the "comments" from value_param() and
> type_param(), so the "comments" are just a derivative and can be
> removed.
>
> As to #2, since no value or type should be printed as the empty
> string, we can safely use the empty string to indicate "no param".
>
> It's not safe. We never prohibited using an empty string - or any object
which prints as an empty string - as a parameter. People who did so will
have those attributes dropped from their output XML files for the affected
instantiations, without them seeing any warning.

I can think of a couple of alternatives. One is to use const char* and pass
NULL for non-parameterized tests. Another is to have a single 'param'
parameter and prefix the value with 't' or 'v' for type- or
value-parameterized test parameters. An empty string would then indicate no
parameter.

If the value itself is the empty string, it shall be printed as two
> double-quotes (i.e. "\"\"" when written as a C string literal), which
> isn't the same as an empty string.
>
> >>
> >> Let me know what you think. I am willing to do the legwork if/when we're
> >> in agreement.
>
> Your plan sounds good!  I'll review it when you are ready.
>
> >>
> >> I'll send you a separate email on the other topic (value- versus type-
> >> cleanup) off list since I didn't totally understand it.
> >>
> >>
> >> -joey
> >
>
>
>
> --
> Zhanyong
>

[wan, 2011-01-20 22:18:36]

2011/1/20 Vlad Losev <vladl@google.com>:
>
>
> 2011/1/20 Zhanyong Wan (λx.x x) <wan@google.com>
>>
>> 2011/1/19 Vlad Losev <vladl@google.com>:
>> >
>> >
>> > 2011/1/19 Joey Oravec <joeyoravec@gmail.com>
>> >>
>> >> 2011/1/19 Vlad Losev <vladl@google.com>
>> >>>
>> >>> I have taken a look. Rietveld is for some reason unable to show
>> >>> side-by-side diffs, so I will comment here. The patch seems OK, with a
>> >>> couple of notes:
>> >>>  * I prefer to scrape the test and test case comments instead of
>> >>> adding
>> >>> more parameters to the the test creation routines. Longer function
>> >>> parameter
>> >>> lists and data member lists make code harder to understand and debug.
>> >>
>> >>
>> >> Did you mean to write scrap (get rid of) the test comment and test case
>> >> comment?
>> >
>> > No, I mean scrape, as in parse and extract information from. Right now
>> > they
>> > are used to carry parameter information for the stdout printer, but can
>> > also
>> > be parsed by the XML printer. Because gtest controls information that
>> > ends
>> > up there, parsing it won't present a problem.
>>
>> Please do not go that route.  The other direction is much more
>> principled and cleaner.
>>
>> >> Right now they're described in gtest-internal.h above
>> >> MakeAndRegisterTestInfo as:
>> >>
>> >> test_case_comment: A comment on the test case that will be included in
>> >> the
>> >> output
>> >> comment: A comment on the test that will be included in the output
>> >>
>> >> Based on the description I would expect these members to hold some
>> >> human-readable description of the testcase and test, but as far as I
>> >> can
>> >> tell they're completely internal and there's no way for a user to fill
>> >> these
>> >> fields when writing a test. Right now they're used for:
>> >>
>> >> type-parameterized tests: set the test_case_comment with "TypeParam =
>> >> ..."
>> >> and leave comment unused
>> >> value-parameterized tests: set the comment with "GetParam = ..." and
>> >> leave
>> >> test_case_comment unused
>> >>
>> >> ...and they're printed by the pretty result printer. Yes I think that's
>> >> redundant, and my patch probably didn't go far enough. I think:
>> >>
>> >> 1. We should replace any existing use of test_case_comment() and
>> >> comment()
>> >> with the appropriate value_param() or type_param(). This is mainly
>> >> going to
>> >> affect pretty result printer.
>> >> 2. As the code stands we cannot avoid the has_X_param boolean because
>> >> that
>> >> lets us distinguish between no param, and a param which printed as a
>> >> zero-length string.
>> >> 3. Finally if test_case_comment() and comment() don't have any use, we
>> >> could get rid of them.
>>
>> I completely agree with you here except #2.  When I asked you to store
>> the text presentation of the value- and type- parameters in TestInfo,
>> I actually meant to replace the *comment() methods -- if necessary, we
>> can always calculate the "comments" from value_param() and
>> type_param(), so the "comments" are just a derivative and can be
>> removed.
>>
>> As to #2, since no value or type should be printed as the empty
>> string, we can safely use the empty string to indicate "no param".
>>
> It's not safe.

Good point.  Joey also explained it off-line to me.

> We never prohibited using an empty string - or any object
> which prints as an empty string - as a parameter. People who did so will
> have those attributes dropped from their output XML files for the affected
> instantiations, without them seeing any warning.

An empty string value won't be a problem, as it will be printed as
"\"\"".  However, as Joey and you pointed out, if a user-defined value
printer prints an empty string, it will be mis-interpreted in my
proposal.  Therefore I take back what I said about #2.

> I can think of a couple of alternatives. One is to use const char* and pass
> NULL for non-parameterized tests. Another is to have a single 'param'
> parameter and prefix the value with 't' or 'v' for type- or
> value-parameterized test parameters. An empty string would then indicate no
> parameter.

I'd prefer to avoid the 't' or 'v' encoding as it's less safe/clear
(the compiler doesn't understand this convention).

Here's another way: use const string* and reserve NULL for "no param".
 It's easier to manage than const char*.

Joey, I'm not concerned with the memory overhead of an additional
bool.  It's the case where the bool is false (no param) but the param
field is not empty that bothers me.  Ideally, we should eliminate such
invalid construct by design.  Are you OK with const std::string*?

>> If the value itself is the empty string, it shall be printed as two
>> double-quotes (i.e. "\"\"" when written as a C string literal), which
>> isn't the same as an empty string.
>>
>> >>
>> >> Let me know what you think. I am willing to do the legwork if/when
>> >> we're
>> >> in agreement.
>>
>> Your plan sounds good!  I'll review it when you are ready.
>>
>> >>
>> >> I'll send you a separate email on the other topic (value- versus type-
>> >> cleanup) off list since I didn't totally understand it.
>> >>
>> >>
>> >> -joey
>> >
>>
>>
>>
>> --
>> Zhanyong
>
>



-- 
Zhanyong

[joeyoravec, 2011-01-27 21:21:30]

2011/1/20 Zhanyong Wan (λx.x x) <wan@google.com>

>  >> >>> I have taken a look. Rietveld is for some reason unable to show
> >> >>> side-by-side diffs, so I will comment here.
>

Fixed. I'm only able to use the rietveld web upload form, and I think I had
set the base URL incorrectly.



> Joey, I'm not concerned with the memory overhead of an additional
> bool.  It's the case where the bool is false (no param) but the param
> field is not empty that bothers me.  Ideally, we should eliminate such
> invalid construct by design.  Are you OK with const std::string*?
>

Sure. This revision eliminates the "bool has_value_param" parameter in favor
of a single parameter which either points to a C-string if the test is
value-parameterized, or is equal to NULL if the test is not
value-parameterized. The object still uses a private bool member internally;
you wanted a simpler interface, but I didn't see any reason to change that
member variable from a simple value-type to dynamic allocation.



> >> Your plan sounds good!  I'll review it when you are ready.
>


Ok, I've gotten the hang of Rietveld so please review the two new patches at
http://codereview.appspot.com/3773042/

Patch1:
 - Tracks the value-parameter or type-parameter as a string
 - Adds value_param="xx" and type_param="yy" to the XML output
 - Modifies pretty result printer to print in the same way (note, function
renamed)

Patch2 (check delta from patch 1):
 - Deletes test_case_comment() and comment() which are now unused

Same as before: use sample1 (normal), sample6 (type-parameterized), and
sample7 (value-parameterized) to verify this works. You'll need to add a
FAIL() to exercise pretty result printer, because it prints nothing for
successful tests.

-joey

[wan, 2011-01-29 08:33:46]

Thanks, Joey.

The patch looks good overall.  It has to be updated to meet our coding
style requirements though.  Also, I prefer to remove the *comment()
fields in the same patch.  While it's cleaner to do it in a separate
step, we don't want to leave gtest in a (even though temporary) state
whether a TestInfo gains more fields than necessary -- internally we
use gtest at head so any change in the memory footprint has the risk
of breaking some existing tests.

Since I'm short in time, I'll just clean up your patch myself and
commit it sometime next week, instead of pointing out all the places
it needs to be tweaked.  (The former is easier for me.)  Thanks,

2011/1/27 Joey Oravec <joeyoravec@gmail.com>:
> 2011/1/20 Zhanyong Wan (λx.x x) <wan@google.com>
>>
>> >> >>> I have taken a look. Rietveld is for some reason unable to show
>> >> >>> side-by-side diffs, so I will comment here.
>
>
> Fixed. I'm only able to use the rietveld web upload form, and I think I had
> set the base URL incorrectly.
>
>
>>
>> Joey, I'm not concerned with the memory overhead of an additional
>> bool.  It's the case where the bool is false (no param) but the param
>> field is not empty that bothers me.  Ideally, we should eliminate such
>> invalid construct by design.  Are you OK with const std::string*?
>
>
> Sure. This revision eliminates the "bool has_value_param" parameter in favor
> of a single parameter which either points to a C-string if the test is
> value-parameterized, or is equal to NULL if the test is not
> value-parameterized. The object still uses a private bool member internally;
> you wanted a simpler interface, but I didn't see any reason to change that
> member variable from a simple value-type to dynamic allocation.
>
>
>>
>> >> Your plan sounds good!  I'll review it when you are ready.
>
>
>
> Ok, I've gotten the hang of Rietveld so please review the two new patches at
> http://codereview.appspot.com/3773042/
>
> Patch1:
>  - Tracks the value-parameter or type-parameter as a string
>  - Adds value_param="xx" and type_param="yy" to the XML output
>  - Modifies pretty result printer to print in the same way (note, function
> renamed)
>
> Patch2 (check delta from patch 1):
>  - Deletes test_case_comment() and comment() which are now unused
>
> Same as before: use sample1 (normal), sample6 (type-parameterized), and
> sample7 (value-parameterized) to verify this works. You'll need to add a
> FAIL() to exercise pretty result printer, because it prints nothing for
> successful tests.
>
> -joey



-- 
Zhanyong

[vladl_google.com, 2011-02-02 03:29:24]

Joey,

This has been added in SVN revision 537. Enjoy!

2011/1/29 Zhanyong Wan (λx.x x) <wan@google.com>

> Thanks, Joey.
>
> The patch looks good overall.  It has to be updated to meet our coding
> style requirements though.  Also, I prefer to remove the *comment()
> fields in the same patch.  While it's cleaner to do it in a separate
> step, we don't want to leave gtest in a (even though temporary) state
> whether a TestInfo gains more fields than necessary -- internally we
> use gtest at head so any change in the memory footprint has the risk
> of breaking some existing tests.
>
> Since I'm short in time, I'll just clean up your patch myself and
> commit it sometime next week, instead of pointing out all the places
> it needs to be tweaked.  (The former is easier for me.)  Thanks,
>
> 2011/1/27 Joey Oravec <joeyoravec@gmail.com>:
> > 2011/1/20 Zhanyong Wan (λx.x x) <wan@google.com>
> >>
> >> >> >>> I have taken a look. Rietveld is for some reason unable to show
> >> >> >>> side-by-side diffs, so I will comment here.
> >
> >
> > Fixed. I'm only able to use the rietveld web upload form, and I think I
> had
> > set the base URL incorrectly.
> >
> >
> >>
> >> Joey, I'm not concerned with the memory overhead of an additional
> >> bool.  It's the case where the bool is false (no param) but the param
> >> field is not empty that bothers me.  Ideally, we should eliminate such
> >> invalid construct by design.  Are you OK with const std::string*?
> >
> >
> > Sure. This revision eliminates the "bool has_value_param" parameter in
> favor
> > of a single parameter which either points to a C-string if the test is
> > value-parameterized, or is equal to NULL if the test is not
> > value-parameterized. The object still uses a private bool member
> internally;
> > you wanted a simpler interface, but I didn't see any reason to change
> that
> > member variable from a simple value-type to dynamic allocation.
> >
> >
> >>
> >> >> Your plan sounds good!  I'll review it when you are ready.
> >
> >
> >
> > Ok, I've gotten the hang of Rietveld so please review the two new patches
> at
> > http://codereview.appspot.com/3773042/
> >
> > Patch1:
> >  - Tracks the value-parameter or type-parameter as a string
> >  - Adds value_param="xx" and type_param="yy" to the XML output
> >  - Modifies pretty result printer to print in the same way (note,
> function
> > renamed)
> >
> > Patch2 (check delta from patch 1):
> >  - Deletes test_case_comment() and comment() which are now unused
> >
> > Same as before: use sample1 (normal), sample6 (type-parameterized), and
> > sample7 (value-parameterized) to verify this works. You'll need to add a
> > FAIL() to exercise pretty result printer, because it prints nothing for
> > successful tests.
> >
> > -joey
>
>
>
> --
> Zhanyong
>

Regards,
Vlad