I'm in favor of (b). The less user-required setup to do the right thing on a test suite, the better IMHO. Those actively trying to make sure one or another c++ library is getting tested will be looking for the output to validate which std c++ lib(s) ran.
-Todd On Wed, Oct 21, 2015 at 3:47 AM, Pavel Labath <lab...@google.com> wrote: > [moving this to lldb-dev for more visibility.] > > Sorry, I was in a hurry yesterday, so I did not explain myself fully. Let > me try to elaborate. > > > What I am trying to avoid here is getting useless noise in build > automation where test cases proclaim their failure, which however tells us > nothing of value as the issue is simply “libc++ not available” vs. “data > formatters broken” That is an ability I would strongly like to preserve > > I agree that we should have an ability to skip libc++ tests if the library > is not available (similarly for go, etc.). However, I believe there should > be as little "magic" in that as possible. Otherwise you run into the > opposite problem where a test passing could mean "everything is ok" or "our > auto-skipping magic has gone wrong", which I would argue is a worse > situation than the first one. > > Currently, we have a lot of magic in here: > - self.build() silently builds an executable with a different library even > though libc++ was requested. (otherwise you wouldn't even be able to list > the modules of the executable) > - the test decides to skip itself without giving any indication about > (sure, it shows up in the "skipped tests" list, but a lot of other stuff > appears there as well. and I would argue that this is a different > situation: usually we do skips based on the OS, architecture, or other > "immutable" characteristics. Presence of libc++ is something that can be > usually changed by simply installing a package.) > > I'd like to propose a few alternative solutions to achieve both of these > objectives: > > a) Use the existing category system for this: libc++ tests get tagged as > such and the user has to explicitly disable this category to avoid running > the tests. Potentially, add some code to detect when the user is running > the test suite without libc++ installed and abort the run with some message > like "No libc++ detected on your system. Please install libc++ or disable > libc++ tests with `--disable-category libc++`". > > I like this solution because it is easily achievable and has no magic > involved. However, it requires an action from the user (which I think is a > good thing, but I see how others may disagree). That's what I meant for > asking about your "use case". Would you be able to fit it inside this > framework? > > b) Use category tagging as in (a), but auto-disable this category when > libc++ is missing and print a big fat warning "No libc++ detected, libc++ > tests will not run". What is different from the current state is that this > libc++ detection would work centrally, which would give us the possibility > to print the warning (e.g. right before the "Ran XXX test suites" message") > I like this a bit less, as it is still automatic, but I am fine with it, > as it would give a clear visual indication of what is happening. > > > What do you think? Do you have another proposal? > > I can implement either of those, but I was to get some feedback first.. > > pl > > > > On 20 October 2015 at 19:05, Enrico Granata <egran...@apple.com> wrote: > >> >> On Oct 20, 2015, at 10:43 AM, Pavel Labath <lab...@google.com> wrote: >> >> Hi Enrico, >> >> Could you explain what was the motivation behind this change? >> >> >> As per title of the commit, in a process that is using a standard c++ >> library other than libc++, these tests are noise - of course the libc++ >> tests aren’t gonna pass if you don’t have libc++ >> >> I am asking because, I have just learned that this commit has caused >> all libc++ tests to be skipped on linux*, silently decreasing test >> coverage on linux. I would like to replace this with some other >> mechanism, which is not prone to accidental silent skips, like a >> dotest flag to skip libc++ or something, but I'd like to understand >> the original motivation first. >> >> pl >> >> * the problem seems to be that on linux, we do not have the list of >> modules until we actually start the process, so this code will not >> find the library, as it runs before that. >> >> >> The solution might then be to run the process, and then >> skip_if_library_missing >> I think we avoid trying to compile the test inferior entirely if we can’t >> find libc++ however, so you might first want to check if a.out exists at >> all, and only then proceed all the way to the first breakpoint being hit >> >> If is considered a bug then >> we can look into that separately, but I'd still like to avoid these >> kinds of skips. >> >> >> What I am trying to avoid here is getting useless noise in build >> automation where test cases proclaim their failure, which however tells us >> nothing of value as the issue is simply “libc++ not available” vs. “data >> formatters broken” >> That is an ability I would strongly like to preserve >> >> >> On 18 September 2015 at 21:12, Enrico Granata via lldb-commits >> <lldb-comm...@lists.llvm.org> wrote: >> >> Author: enrico >> Date: Fri Sep 18 15:12:52 2015 >> New Revision: 248028 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=248028&view=rev >> Log: >> Make libc++ tests skip themselves if libc++ is not actually loaded in the >> target >> >> >> Modified: >> >> >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py >> >> >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/iterator/TestDataFormatterLibccIterator.py >> >> >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py >> >> >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py >> >> >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/multimap/TestDataFormatterLibccMultiMap.py >> >> >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/multiset/TestDataFormatterLibcxxMultiSet.py >> >> >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py >> >> >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py >> >> >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/unordered/TestDataFormatterUnordered.py >> >> >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/vbool/TestDataFormatterLibcxxVBool.py >> >> >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/vector/TestDataFormatterLibcxxVector.py >> lldb/trunk/test/lldbutil.py >> >> Modified: >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py >> (original) >> +++ >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/initializerlist/TestInitializerList.py >> Fri Sep 18 15:12:52 2015 >> @@ -36,6 +36,8 @@ class InitializerListTestCase(TestBase): >> """Test that that file and class static variables display >> correctly.""" >> self.runCmd("file a.out", CURRENT_EXECUTABLE_SET) >> >> + lldbutil.skip_if_library_missing(self, self.target(), >> lldbutil.PrintableRegex("libc\+\+")) >> + >> bkpt = >> self.target().FindBreakpointByID(lldbutil.run_break_set_by_source_regexp >> (self, "Set break point at this line.")) >> >> self.runCmd("run", RUN_SUCCEEDED) >> >> Modified: >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/iterator/TestDataFormatterLibccIterator.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/iterator/TestDataFormatterLibccIterator.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/iterator/TestDataFormatterLibccIterator.py >> (original) >> +++ >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/iterator/TestDataFormatterLibccIterator.py >> Fri Sep 18 15:12:52 2015 >> @@ -37,6 +37,8 @@ class LibcxxIteratorDataFormatterTestCas >> """Test that libc++ iterators format properly.""" >> self.runCmd("file a.out", CURRENT_EXECUTABLE_SET) >> >> + lldbutil.skip_if_library_missing(self, self.target(), >> lldbutil.PrintableRegex("libc\+\+")) >> + >> lldbutil.run_break_set_by_file_and_line (self, "main.cpp", >> self.line, num_expected_locations=-1) >> >> self.runCmd("run", RUN_SUCCEEDED) >> >> Modified: >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py >> (original) >> +++ >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py >> Fri Sep 18 15:12:52 2015 >> @@ -2,7 +2,7 @@ >> Test lldb data formatter subsystem. >> """ >> >> -import os, time >> +import os, time, re >> import unittest2 >> import lldb >> from lldbtest import * >> @@ -39,6 +39,8 @@ class LibcxxListDataFormatterTestCase(Te >> def data_formatter_commands(self): >> """Test that that file and class static variables display >> correctly.""" >> self.runCmd("file a.out", CURRENT_EXECUTABLE_SET) >> + >> + lldbutil.skip_if_library_missing(self, self.target(), >> lldbutil.PrintableRegex("libc\+\+")) >> >> lldbutil.run_break_set_by_file_and_line (self, "main.cpp", >> self.line, num_expected_locations=-1) >> lldbutil.run_break_set_by_file_and_line (self, "main.cpp", >> self.line2, num_expected_locations=-1) >> >> Modified: >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py >> (original) >> +++ >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py >> Fri Sep 18 15:12:52 2015 >> @@ -35,6 +35,8 @@ class LibcxxMapDataFormatterTestCase(Tes >> """Test that that file and class static variables display >> correctly.""" >> self.runCmd("file a.out", CURRENT_EXECUTABLE_SET) >> >> + lldbutil.skip_if_library_missing(self, self.target(), >> lldbutil.PrintableRegex("libc\+\+")) >> + >> bkpt = >> self.target().FindBreakpointByID(lldbutil.run_break_set_by_source_regexp >> (self, "Set break point at this line.")) >> >> self.runCmd("run", RUN_SUCCEEDED) >> >> Modified: >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/multimap/TestDataFormatterLibccMultiMap.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/multimap/TestDataFormatterLibccMultiMap.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/multimap/TestDataFormatterLibccMultiMap.py >> (original) >> +++ >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/multimap/TestDataFormatterLibccMultiMap.py >> Fri Sep 18 15:12:52 2015 >> @@ -34,6 +34,8 @@ class LibcxxMultiMapDataFormatterTestCas >> def data_formatter_commands(self): >> """Test that that file and class static variables display >> correctly.""" >> self.runCmd("file a.out", CURRENT_EXECUTABLE_SET) >> + >> + lldbutil.skip_if_library_missing(self, self.target(), >> lldbutil.PrintableRegex("libc\+\+")) >> >> bkpt = >> self.target().FindBreakpointByID(lldbutil.run_break_set_by_source_regexp >> (self, "Set break point at this line.")) >> >> >> Modified: >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/multiset/TestDataFormatterLibcxxMultiSet.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/multiset/TestDataFormatterLibcxxMultiSet.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/multiset/TestDataFormatterLibcxxMultiSet.py >> (original) >> +++ >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/multiset/TestDataFormatterLibcxxMultiSet.py >> Fri Sep 18 15:12:52 2015 >> @@ -34,6 +34,8 @@ class LibcxxMultiSetDataFormatterTestCas >> def data_formatter_commands(self): >> """Test that that file and class static variables display >> correctly.""" >> self.runCmd("file a.out", CURRENT_EXECUTABLE_SET) >> + >> + lldbutil.skip_if_library_missing(self, self.target(), >> lldbutil.PrintableRegex("libc\+\+")) >> >> bkpt = >> self.target().FindBreakpointByID(lldbutil.run_break_set_by_source_regexp >> (self, "Set break point at this line.")) >> >> >> Modified: >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py >> (original) >> +++ >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py >> Fri Sep 18 15:12:52 2015 >> @@ -34,6 +34,8 @@ class LibcxxSetDataFormatterTestCase(Tes >> def data_formatter_commands(self): >> """Test that that file and class static variables display >> correctly.""" >> self.runCmd("file a.out", CURRENT_EXECUTABLE_SET) >> + >> + lldbutil.skip_if_library_missing(self, self.target(), >> lldbutil.PrintableRegex("libc\+\+")) >> >> bkpt = >> self.target().FindBreakpointByID(lldbutil.run_break_set_by_source_regexp >> (self, "Set break point at this line.")) >> >> >> Modified: >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py >> (original) >> +++ >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py >> Fri Sep 18 15:12:52 2015 >> @@ -38,6 +38,8 @@ class LibcxxStringDataFormatterTestCase( >> """Test that that file and class static variables display >> correctly.""" >> self.runCmd("file a.out", CURRENT_EXECUTABLE_SET) >> >> + lldbutil.skip_if_library_missing(self, self.target(), >> lldbutil.PrintableRegex("libc\+\+")) >> + >> lldbutil.run_break_set_by_file_and_line (self, "main.cpp", >> self.line, num_expected_locations=-1) >> >> self.runCmd("run", RUN_SUCCEEDED) >> >> Modified: >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/unordered/TestDataFormatterUnordered.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/unordered/TestDataFormatterUnordered.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/unordered/TestDataFormatterUnordered.py >> (original) >> +++ >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/unordered/TestDataFormatterUnordered.py >> Fri Sep 18 15:12:52 2015 >> @@ -39,6 +39,8 @@ class LibcxxUnorderedDataFormatterTestCa >> """Test that that file and class static variables display >> correctly.""" >> self.runCmd("file a.out", CURRENT_EXECUTABLE_SET) >> >> + lldbutil.skip_if_library_missing(self, self.target(), >> lldbutil.PrintableRegex("libc\+\+")) >> + >> lldbutil.run_break_set_by_source_regexp (self, "Set break point >> at this line.") >> >> self.runCmd("run", RUN_SUCCEEDED) >> >> Modified: >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/vbool/TestDataFormatterLibcxxVBool.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/vbool/TestDataFormatterLibcxxVBool.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/vbool/TestDataFormatterLibcxxVBool.py >> (original) >> +++ >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/vbool/TestDataFormatterLibcxxVBool.py >> Fri Sep 18 15:12:52 2015 >> @@ -37,6 +37,8 @@ class LibcxxVBoolDataFormatterTestCase(T >> """Test that that file and class static variables display >> correctly.""" >> self.runCmd("file a.out", CURRENT_EXECUTABLE_SET) >> >> + lldbutil.skip_if_library_missing(self, self.target(), >> lldbutil.PrintableRegex("libc\+\+")) >> + >> lldbutil.run_break_set_by_file_and_line (self, "main.cpp", >> self.line, num_expected_locations=-1) >> >> self.runCmd("run", RUN_SUCCEEDED) >> >> Modified: >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/vector/TestDataFormatterLibcxxVector.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/vector/TestDataFormatterLibcxxVector.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/vector/TestDataFormatterLibcxxVector.py >> (original) >> +++ >> lldb/trunk/test/functionalities/data-formatter/data-formatter-stl/libcxx/vector/TestDataFormatterLibcxxVector.py >> Fri Sep 18 15:12:52 2015 >> @@ -34,6 +34,8 @@ class LibcxxVectorDataFormatterTestCase( >> def data_formatter_commands(self): >> """Test that that file and class static variables display >> correctly.""" >> self.runCmd("file a.out", CURRENT_EXECUTABLE_SET) >> + >> + lldbutil.skip_if_library_missing(self, self.target(), >> lldbutil.PrintableRegex("libc\+\+")) >> >> bkpt = >> self.target().FindBreakpointByID(lldbutil.run_break_set_by_source_regexp >> (self, "break here")) >> >> >> Modified: lldb/trunk/test/lldbutil.py >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/test/lldbutil.py?rev=248028&r1=248027&r2=248028&view=diff >> >> ============================================================================== >> --- lldb/trunk/test/lldbutil.py (original) >> +++ lldb/trunk/test/lldbutil.py Fri Sep 18 15:12:52 2015 >> @@ -7,6 +7,7 @@ They can also be useful for general purp >> import lldb >> import os, sys >> import StringIO >> +import re >> >> # =================================================== >> # Utilities for locating/checking executable programs >> @@ -957,3 +958,38 @@ def get_signal_number(signal_name): >> return signal_number >> # No remote platform; fall back to using local python signals. >> return getattr(signal, signal_name) >> + >> +class PrintableRegex(object): >> + def __init__(self, text): >> + self.regex = re.compile(text) >> + self.text = text >> + >> + def match(self, str): >> + return self.regex.match(str) >> + >> + def __str__(self): >> + return "%s" % (self.text) >> + >> + def __repr__(self): >> + return "re.compile(%s) -> %s" % (self.text, self.regex) >> + >> +def skip_if_callable(test, callable, reason): >> + if callable(test) == True: >> + test.skipTest(reason) >> + return True >> + return False >> + >> +def skip_if_library_missing(test, target, library): >> + def find_library(target, library): >> + for module in target.modules: >> + filename = module.file.GetFilename() >> + if isinstance(library, str): >> + if library == filename: >> + return False >> + elif hasattr(library, 'match'): >> + if library.match(filename): >> + return False >> + return True >> + def find_library_callable(test): >> + return find_library(target, library) >> + return skip_if_callable(test, find_library_callable, "could not find >> library matching '%s' in target %s" % (library, target)) >> >> >> _______________________________________________ >> lldb-commits mailing list >> lldb-comm...@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits >> >> >> >> Thanks, >> *- Enrico* >> 📩 egranata@.com ☎️ 27683 >> >> > -- -Todd
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
