llvm dev - Mar 2018 - [RFC] Updating googletest to non-release tagged version

On Thu, Mar 15, 2018 at 9:09 PM, David Blaikie via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> +Chandler who might have some thoughts on this.
>
> Could you provide an example here of the motivation for the feature
you're
> missing? Might help motivate the discussion (and/or we'll end up
nitpicking
> how it could be done differently without that feature... - which is sort of
> where I'm going with this. Combinatorial test case expansion does seem
a bit
> suspicious to me - I'd hope we could pick a few examples from the
various
> equivalence classes & that would suffice?)
>
> On Mon, Mar 12, 2018 at 9:01 AM James Henderson via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>>
>> Hi all,
>>
>> I'm currently writing some unit tests for some debug line error
handling
>> code I'm working on (see e.g. https://reviews.llvm.org/D44382), and
I just
>> ran into an annoying disabled feature in gtest, specifically the
"Combine"
>> feature for use in combinatorially generating parameterised tests. A
FIXME
>> comment in ProfileData\CoverageMappingTest.cpp suggests that I'm
not the
>> only one to have tried and discovered that they cannot use this
feature. The
>> problem is that the version of googletest (v 1.8.0, released in Aug
2016) in
>> the LLVM tree requires TR1 tuple support for this feature, which is not
>> really supported in recent compilers, and has been explicitly disabled
in
>> our googletest CMakeLists.txt, thus disabling "Combine".
>>
>> I did a bit of looking around, and v 1.8.0 is indeed the last
officially
>> tagged release of googletest. However, there has been a lot of
development
>> on the framework since that point, including a fix to enable use of
Combine
>> with std::tuple-supporting compilers. There have been a number of issue
>> raised on the googletest issue tracker (see e.g.
>> https://github.com/google/googletest/issues/1467 or
>> https://github.com/google/googletest/issues/1079) asking about a 1.9.0
>> release, and there has been zero response from anybody answering the
query
>> of when/if it will happen. In the meantime, the last release gets older
and
>> crustier...
>>
>> I'd therefore like to propose something that might be seen as
slightly
>> controversial: update to use ToT googletest (or at least some
reasonably
>> recent version of master), at least until a new release is created.
>>
>> Thoughts?
This sounds strange.

I've been using googletest-1.8.0 in my project, with fresh compilers
(clang, gcc - https://godbolt.org/g/jg4tFG - tr1/tuple works)
and not once did i have any issues with unavailability of Combine,
even though i do use it.

Debian sid:
$ dpkg -S tr1/tuple
libstdc++-7-dev:amd64: /usr/include/c++/7/tr1/tuple
libstdc++-5-dev:amd64: /usr/include/c++/5/tr1/tuple
libstdc++-6-dev:amd64: /usr/include/c++/6/tr1/tuple

So "which is not really supported in recent compilers" part is at
least too broad.

Roman.
>> James
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

Thanks. The motivating example can be seen in this review:
https://reviews.llvm.org/D44382.

In that review, I am unit testing .debug_line parsing, specifically, the
behaviour when the parser is fed a malformed section. Most of the code
under test goes through some slight variations in the code path, depending
on a) the DWARF version (interesting cases are 3, 4 and 5), and b) whether
the DWARF is 32-bit DWARF or 64-bit DWARF. So I have two axes, with
multiple values on each. The end "symptoms" being tested aren't
much
different in each case, so it makes sense to share the test code as much as
possible. Without Combine, I have to do the following:

INSTANTIATE_TEST_CASE_P(
    LineTableTestParams, DebugLineParameterisedFixture,
    Values(std::make_pair(2, DWARF32), std::make_pair(3, DWARF32),
           std::make_pair(4, DWARF32), std::make_pair(5, DWARF32),
           std::make_pair(2, DWARF64), std::make_pair(3, DWARF64),
           std::make_pair(4, DWARF64), std::make_pair(5, DWARF64)));

I realised whilst typing this out, that I don't need a distinction between
v2 and v3. However, there's still 6 different combinations, which I have to
explicitly list. Combine would, by my understanding, allow me to do
something like "Combine(Values(3, 4, 5), Values(DWARF32, DWARF64))".

As to why it doesn't work in 1.8, it's because LLVM has explicitly
overridden the GTEST_HAS_TR1_TUPLE define (see
utils\unittest\CMakeLists.txt and r316798). The short-story is that it's
because recent MSVC compilers (sorry, I guess I was too broad in my
original statement) have started issuing TR1 deprecation warnings, combined
with C++11 not being detected properly.

On 15 March 2018 at 18:31, Roman Lebedev <lebedev.ri at gmail.com> wrote:
> On Thu, Mar 15, 2018 at 9:09 PM, David Blaikie via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > +Chandler who might have some thoughts on this.
> >
> > Could you provide an example here of the motivation for the feature
> you're
> > missing? Might help motivate the discussion (and/or we'll end up
> nitpicking
> > how it could be done differently without that feature... - which is
sort
> of
> > where I'm going with this. Combinatorial test case expansion does
seem a
> bit
> > suspicious to me - I'd hope we could pick a few examples from the
various
> > equivalence classes & that would suffice?)
> >
> > On Mon, Mar 12, 2018 at 9:01 AM James Henderson via llvm-dev
> > <llvm-dev at lists.llvm.org> wrote:
> >>
> >> Hi all,
> >>
> >> I'm currently writing some unit tests for some debug line
error handling
> >> code I'm working on (see e.g.
https://reviews.llvm.org/D44382), and I
> just
> >> ran into an annoying disabled feature in gtest, specifically the
> "Combine"
> >> feature for use in combinatorially generating parameterised tests.
A
> FIXME
> >> comment in ProfileData\CoverageMappingTest.cpp suggests that
I'm not
> the
> >> only one to have tried and discovered that they cannot use this
> feature. The
> >> problem is that the version of googletest (v 1.8.0, released in
Aug
> 2016) in
> >> the LLVM tree requires TR1 tuple support for this feature, which
is not
> >> really supported in recent compilers, and has been explicitly
disabled
> in
> >> our googletest CMakeLists.txt, thus disabling "Combine".
> >>
> >> I did a bit of looking around, and v 1.8.0 is indeed the last
officially
> >> tagged release of googletest. However, there has been a lot of
> development
> >> on the framework since that point, including a fix to enable use
of
> Combine
> >> with std::tuple-supporting compilers. There have been a number of
issue
> >> raised on the googletest issue tracker (see e.g.
> >> https://github.com/google/googletest/issues/1467 or
> >> https://github.com/google/googletest/issues/1079) asking about a
1.9.0
> >> release, and there has been zero response from anybody answering
the
> query
> >> of when/if it will happen. In the meantime, the last release gets
older
> and
> >> crustier...
> >>
> >> I'd therefore like to propose something that might be seen as
slightly
> >> controversial: update to use ToT googletest (or at least some
reasonably
> >> recent version of master), at least until a new release is
created.
> >>
> >> Thoughts?
> This sounds strange.
>
> I've been using googletest-1.8.0 in my project, with fresh compilers
> (clang, gcc - https://godbolt.org/g/jg4tFG - tr1/tuple works)
> and not once did i have any issues with unavailability of Combine,
> even though i do use it.
>
> Debian sid:
> $ dpkg -S tr1/tuple
> libstdc++-7-dev:amd64: /usr/include/c++/7/tr1/tuple
> libstdc++-5-dev:amd64: /usr/include/c++/5/tr1/tuple
> libstdc++-6-dev:amd64: /usr/include/c++/6/tr1/tuple
>
> So "which is not really supported in recent compilers" part is at
> least too broad.
>
> Roman.
>
> >> James
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> llvm-dev at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180316/4224fcae/attachment.html>

On Fri, Mar 16, 2018 at 2:41 AM James Henderson <
jh7370.2008 at my.bristol.ac.uk> wrote:
> Thanks. The motivating example can be seen in this review:
> https://reviews.llvm.org/D44382.
>
> In that review, I am unit testing .debug_line parsing, specifically, the
> behaviour when the parser is fed a malformed section. Most of the code
> under test goes through some slight variations in the code path, depending
> on a) the DWARF version (interesting cases are 3, 4 and 5), and b) whether
> the DWARF is 32-bit DWARF or 64-bit DWARF. So I have two axes, with
> multiple values on each. The end "symptoms" being tested
aren't much
> different in each case, so it makes sense to share the test code as much as
> possible. Without Combine, I have to do the following:
>
> INSTANTIATE_TEST_CASE_P(
>     LineTableTestParams, DebugLineParameterisedFixture,
>     Values(std::make_pair(2, DWARF32), std::make_pair(3, DWARF32),
>            std::make_pair(4, DWARF32), std::make_pair(5, DWARF32),
>            std::make_pair(2, DWARF64), std::make_pair(3, DWARF64),
>            std::make_pair(4, DWARF64), std::make_pair(5, DWARF64)));
>
> I realised whilst typing this out, that I don't need a distinction
between
> v2 and v3. However, there's still 6 different combinations, which I
have to
> explicitly list. Combine would, by my understanding, allow me to do
> something like "Combine(Values(3, 4, 5), Values(DWARF32,
DWARF64))".
>
Is it especially valuable to test all 6? Or would sufficient
coverage/confidence be achieved by testing, say, 3+32, 4+64, and 5+64? (ie:
there's at least one test for each of the values, but not for each
combination) - or is there interesting logic in the implementation that's
different for each combination/pair?

>
> As to why it doesn't work in 1.8, it's because LLVM has explicitly
> overridden the GTEST_HAS_TR1_TUPLE define (see
> utils\unittest\CMakeLists.txt and r316798). The short-story is that
it's
> because recent MSVC compilers (sorry, I guess I was too broad in my
> original statement) have started issuing TR1 deprecation warnings, combined
> with C++11 not being detected properly.
>
> On 15 March 2018 at 18:31, Roman Lebedev <lebedev.ri at gmail.com>
wrote:
>
>> On Thu, Mar 15, 2018 at 9:09 PM, David Blaikie via llvm-dev
>> <llvm-dev at lists.llvm.org> wrote:
>> > +Chandler who might have some thoughts on this.
>> >
>> > Could you provide an example here of the motivation for the
feature
>> you're
>> > missing? Might help motivate the discussion (and/or we'll end
up
>> nitpicking
>> > how it could be done differently without that feature... - which
is
>> sort of
>> > where I'm going with this. Combinatorial test case expansion
does seem
>> a bit
>> > suspicious to me - I'd hope we could pick a few examples from
the
>> various
>> > equivalence classes & that would suffice?)
>> >
>> > On Mon, Mar 12, 2018 at 9:01 AM James Henderson via llvm-dev
>> > <llvm-dev at lists.llvm.org> wrote:
>> >>
>> >> Hi all,
>> >>
>> >> I'm currently writing some unit tests for some debug line
error
>> handling
>> >> code I'm working on (see e.g.
https://reviews.llvm.org/D44382), and I
>> just
>> >> ran into an annoying disabled feature in gtest, specifically
the
>> "Combine"
>> >> feature for use in combinatorially generating parameterised
tests. A
>> FIXME
>> >> comment in ProfileData\CoverageMappingTest.cpp suggests that
I'm not
>> the
>> >> only one to have tried and discovered that they cannot use
this
>> feature. The
>> >> problem is that the version of googletest (v 1.8.0, released
in Aug
>> 2016) in
>> >> the LLVM tree requires TR1 tuple support for this feature,
which is not
>> >> really supported in recent compilers, and has been explicitly
disabled
>> in
>> >> our googletest CMakeLists.txt, thus disabling
"Combine".
>> >>
>> >> I did a bit of looking around, and v 1.8.0 is indeed the last
>> officially
>> >> tagged release of googletest. However, there has been a lot of
>> development
>> >> on the framework since that point, including a fix to enable
use of
>> Combine
>> >> with std::tuple-supporting compilers. There have been a number
of issue
>> >> raised on the googletest issue tracker (see e.g.
>> >> https://github.com/google/googletest/issues/1467 or
>> >> https://github.com/google/googletest/issues/1079) asking about
a 1.9.0
>> >> release, and there has been zero response from anybody
answering the
>> query
>> >> of when/if it will happen. In the meantime, the last release
gets
>> older and
>> >> crustier...
>> >>
>> >> I'd therefore like to propose something that might be seen
as slightly
>> >> controversial: update to use ToT googletest (or at least some
>> reasonably
>> >> recent version of master), at least until a new release is
created.
>> >>
>> >> Thoughts?
>> This sounds strange.
>>
>> I've been using googletest-1.8.0 in my project, with fresh
compilers
>> (clang, gcc - https://godbolt.org/g/jg4tFG - tr1/tuple works)
>> and not once did i have any issues with unavailability of Combine,
>> even though i do use it.
>>
>> Debian sid:
>> $ dpkg -S tr1/tuple
>> libstdc++-7-dev:amd64: /usr/include/c++/7/tr1/tuple
>> libstdc++-5-dev:amd64: /usr/include/c++/5/tr1/tuple
>> libstdc++-6-dev:amd64: /usr/include/c++/6/tr1/tuple
>>
>> So "which is not really supported in recent compilers" part
is at
>> least too broad.
>>
>> Roman.
>>
>> >> James
>> >> _______________________________________________
>> >> LLVM Developers mailing list
>> >> llvm-dev at lists.llvm.org
>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> >
>> >
>> > _______________________________________________
>> > LLVM Developers mailing list
>> > llvm-dev at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180319/8f8f5d8a/attachment.html>