Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
labath added a comment. In https://reviews.llvm.org/D24629#550841, @fjricci wrote: > In https://reviews.llvm.org/D24629#550823, @tfiala wrote: > > > > > There is no reasonable thing we can base the expectation as the exact > > > > same device with a different cpu revision could support watchpoints > > > > just fine, so we could just define the list of these tests externally > > > > (in this case, I would probably annotate them with the watchpoint > > > > category and then do the skips based on categories instead). > > > > > > > > > > > Tangential: most chips I've worked on that had hardware watchpoint support > > had an instruction that could be called to find out if such a feature > > exists. I think ARM does this. I would think we could expose an API that > > says whether watchpoints are supported or not, and use that info in LLDB > > and the test suite to enable or disable them. > > > I believe that PTRACE_GETHBPREGS with a value of 0 returns the hardware > stoppoint info on arm, and the byte representing the number of available > hardware watchpoints will be 0 if they aren't supported. Not sure if there's > a simpler way. It's a bit trickier than that. In some cases that call will still return non-zero as the number of supported watchpoints, but the "watchpoint size" field will be zero, and it will still mean that watchpoints don't work. This is probably a kernel bug, though it is pretty easy to work around. The more boring part would be plumbing that information all the way to the test suite - Nothing that can't be done, it's just a bit laborious, so I haven't done that yet. Repository: rL LLVM https://reviews.llvm.org/D24629 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
This revision was automatically updated to reflect the committed changes.
Closed by commit rL282298: Allow for tests to be disabled at runtime (authored
by fjricci).
Changed prior to commit:
https://reviews.llvm.org/D24629?vs=71651=72360#toc
Repository:
rL LLVM
https://reviews.llvm.org/D24629
Files:
lldb/trunk/packages/Python/lldbsuite/test/configuration.py
lldb/trunk/packages/Python/lldbsuite/test/dotest.py
lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py
lldb/trunk/packages/Python/lldbsuite/test/test_result.py
Index: lldb/trunk/packages/Python/lldbsuite/test/configuration.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/configuration.py
+++ lldb/trunk/packages/Python/lldbsuite/test/configuration.py
@@ -101,6 +101,12 @@
# our test cases.
regexp = None
+# Sets of tests which are excluded at runtime
+skip_files = None
+skip_methods = None
+xfail_files = None
+xfail_methods = None
+
# By default, recorded session info for errored/failed test are dumped into its
# own file under a session directory named after the timestamp of the test suite
# run. Use '-s session-dir-name' to specify a specific dir name.
Index: lldb/trunk/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/dotest.py
+++ lldb/trunk/packages/Python/lldbsuite/test/dotest.py
@@ -26,6 +26,7 @@
import os
import errno
import platform
+import re
import signal
import socket
import subprocess
@@ -202,6 +203,48 @@
sys.exit(0)
+def parseExclusion(exclusion_file):
+"""Parse an exclusion file, of the following format, where
+ 'skip files', 'skip methods', 'xfail files', and 'xfail methods'
+ are the possible list heading values:
+
+ skip files
+
+
+
+ xfail methods
+
+"""
+excl_type = None
+case_type = None
+
+with open(exclusion_file) as f:
+for line in f:
+if not excl_type:
+[excl_type, case_type] = line.split()
+continue
+
+line = line.strip()
+if not line:
+excl_type = None
+elif excl_type == 'skip' and case_type == 'files':
+if not configuration.skip_files:
+configuration.skip_files = []
+configuration.skip_files.append(line)
+elif excl_type == 'skip' and case_type == 'methods':
+if not configuration.skip_methods:
+configuration.skip_methods = []
+configuration.skip_methods.append(line)
+elif excl_type == 'xfail' and case_type == 'files':
+if not configuration.xfail_files:
+configuration.xfail_files = []
+configuration.xfail_files.append(line)
+elif excl_type == 'xfail' and case_type == 'methods':
+if not configuration.xfail_methods:
+configuration.xfail_methods = []
+configuration.xfail_methods.append(line)
+
+
def parseOptionsAndInitTestdirs():
"""Initialize the list of directories containing our unittest scripts.
@@ -331,6 +374,9 @@
if args.executable:
lldbtest_config.lldbExec = os.path.realpath(args.executable)
+if args.excluded:
+parseExclusion(args.excluded)
+
if args.p:
if args.p.startswith('-'):
usage(parser)
@@ -749,11 +795,15 @@
def visit_file(dir, name):
# Try to match the regexp pattern, if specified.
if configuration.regexp:
-import re
if not re.search(configuration.regexp, name):
# We didn't match the regex, we're done.
return
+if configuration.skip_files:
+for file_regexp in configuration.skip_files:
+if re.search(file_regexp, name):
+return
+
# We found a match for our test. Add it to the suite.
# Update the sys.path first.
Index: lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py
+++ lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py
@@ -96,6 +96,9 @@
'-p',
metavar='pattern',
help='Specify a regexp filename pattern for inclusion in the test suite')
+group.add_argument('--excluded', metavar='exclusion-file', help=textwrap.dedent(
+'''Specify a file for tests to exclude. File should contain lists of regular expressions for test files or methods,
+with each list under a matching header (xfail files, xfail methods, skip files, skip methods)'''))
group.add_argument(
'-G',
'--category',
Index: lldb/trunk/packages/Python/lldbsuite/test/test_result.py
===
Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
jingham added a subscriber: jingham. jingham added a comment. As long as the only way you can specify the black-list is explicitly on the command line, I think this is fine. There should never be implicit searches for a backlist file. You must have to supply it each time you run the testsuite. That way somebody would have to willfully decide not to run the full testsuite on their patch, and that's a human not a tech problem, since they could just as well check it in with failures they are ignoring, and not need this fancy mechanism... https://reviews.llvm.org/D24629 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
fjricci added a comment. Ok. Barring objections from anyone else, I'll merge this later on today then, with the understanding that if it causes issues like the ones you describe, it should be reverted. https://reviews.llvm.org/D24629 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
fjricci added a comment. In https://reviews.llvm.org/D24629#550823, @tfiala wrote: > > > There is no reasonable thing we can base the expectation as the exact > > > same device with a different cpu revision could support watchpoints just > > > fine, so we could just define the list of these tests externally (in this > > > case, I would probably annotate them with the watchpoint category and > > > then do the skips based on categories instead). > > > > > > Tangential: most chips I've worked on that had hardware watchpoint support > had an instruction that could be called to find out if such a feature exists. > I think ARM does this. I would think we could expose an API that says > whether watchpoints are supported or not, and use that info in LLDB and the > test suite to enable or disable them. I believe that PTRACE_GETHBPREGS with a value of 0 returns that hardware stoppoint info on arm, and the byte representing the number of available hardware watchpoints will be 0 if they aren't supported. Not sure if there's a simpler way. https://reviews.llvm.org/D24629 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
tfiala accepted this revision. tfiala added a comment. This revision is now accepted and ready to land. I am accepting this with one strong reservation which I will explicitly call out here: - If somebody checks in changes that are broken, and claims they missed it because they have an xfail exclusion file and didn't catch it, I will rip this out. If the xfails are hard to setup, it is likely that this is a code smell for needing better decorators to more precisely home in on the cases that are failing. Often times version checks are helpful. I do get the utility this would afford for bring-up of different scenarios, though. Hence I see that being useful enough to have it as an escape hatch. Comment at: packages/Python/lldbsuite/test/configuration.py:107-108 @@ +106,4 @@ +skip_methods = None +xfail_files = None +xfail_methods = None + The skip seems okay. The xfail seems *very* dangerous. Nobody else is going to get these xfails. We're setting ourselves up for having people check in tests that are broken. It allows for a workflow where the user "thinks they're done", when they're not. https://reviews.llvm.org/D24629 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
tfiala added a comment. > > There is no reasonable thing we can base the expectation as the exact same > > device with a different cpu revision could support watchpoints just fine, > > so we could just define the list of these tests externally (in this case, I > > would probably annotate them with the watchpoint category and then do the > > skips based on categories instead). > Tangential: most chips I've worked on that had hardware watchpoint support had an instruction that could be called to find out if such a feature exists. I think ARM does this. I would think we could expose an API that says whether watchpoints are supported or not, and use that info in LLDB and the test suite to enable or disable them. I'll look at the rest of the change here. I'm not opposed to the general idea, although if it encourages people to skip running tests, then check in code that breaks those tests, "unbeknownst to them" (* only because they were intentionally not running them), then I'd say that's bad news. https://reviews.llvm.org/D24629 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
fjricci added a comment. I do understand the complexity problem, and it was one of my concerns with this as well. For my cases, the complexity here is significantly less than the alternatives, but I also do understand if you don't think that's generally true. It probably comes down to how often we think that people are running the test suite in cases where this sort of functionality would be useful. I don't really have a good sense for how other people tend to use the test suite, so I'm personally not sure. For our case, it's a big deal, but if we're the only people who this patch helps, I know it doesn't make sense to merge it. https://reviews.llvm.org/D24629 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
fjricci updated this revision to Diff 71651.
fjricci added a comment.
Refactor re
https://reviews.llvm.org/D24629
Files:
packages/Python/lldbsuite/test/configuration.py
packages/Python/lldbsuite/test/dotest.py
packages/Python/lldbsuite/test/dotest_args.py
packages/Python/lldbsuite/test/test_result.py
Index: packages/Python/lldbsuite/test/test_result.py
===
--- packages/Python/lldbsuite/test/test_result.py
+++ packages/Python/lldbsuite/test/test_result.py
@@ -18,6 +18,8 @@
# Third-party modules
import unittest2
+from unittest2.util import strclass
+
# LLDB Modules
from . import configuration
from lldbsuite.test_event.event_builder import EventBuilder
@@ -124,10 +126,23 @@
test,
test._testMethodName).__func__.__unittest_skip_why__ = "test case does not fall in any category of interest for this run"
+def checkExclusion(self, exclusion_list, name):
+if exclusion_list:
+import re
+for item in exclusion_list:
+if re.search(item, name):
+return True
+return False
+
def startTest(self, test):
if configuration.shouldSkipBecauseOfCategories(
self.getCategoriesForTest(test)):
self.hardMarkAsSkipped(test)
+if self.checkExclusion(
+configuration.skip_methods,
+test._testMethodName):
+self.hardMarkAsSkipped(test)
+
configuration.setCrashInfoHook(
"%s at %s" %
(str(test), inspect.getfile(
@@ -145,6 +160,15 @@
EventBuilder.event_for_start(test))
def addSuccess(self, test):
+if self.checkExclusion(
+configuration.xfail_files,
+strclass(
+test.__class__)) or self.checkExclusion(
+configuration.xfail_methods,
+test._testMethodName):
+self.addUnexpectedSuccess(test, None)
+return
+
super(LLDBTestResult, self).addSuccess(test)
if configuration.parsable:
self.stream.write(
@@ -214,6 +238,15 @@
test, err))
def addFailure(self, test, err):
+if self.checkExclusion(
+configuration.xfail_files,
+strclass(
+test.__class__)) or self.checkExclusion(
+configuration.xfail_methods,
+test._testMethodName):
+self.addExpectedFailure(test, err, None)
+return
+
configuration.sdir_has_content = True
super(LLDBTestResult, self).addFailure(test, err)
method = getattr(test, "markFailure", None)
Index: packages/Python/lldbsuite/test/dotest_args.py
===
--- packages/Python/lldbsuite/test/dotest_args.py
+++ packages/Python/lldbsuite/test/dotest_args.py
@@ -96,6 +96,9 @@
'-p',
metavar='pattern',
help='Specify a regexp filename pattern for inclusion in the test suite')
+group.add_argument('--excluded', metavar='exclusion-file', help=textwrap.dedent(
+'''Specify a file for tests to exclude. File should contain lists of regular expressions for test files or methods,
+with each list under a matching header (xfail files, xfail methods, skip files, skip methods)'''))
group.add_argument(
'-G',
'--category',
Index: packages/Python/lldbsuite/test/dotest.py
===
--- packages/Python/lldbsuite/test/dotest.py
+++ packages/Python/lldbsuite/test/dotest.py
@@ -26,6 +26,7 @@
import os
import errno
import platform
+import re
import signal
import socket
import subprocess
@@ -202,6 +203,48 @@
sys.exit(0)
+def parseExclusion(exclusion_file):
+"""Parse an exclusion file, of the following format, where
+ 'skip files', 'skip methods', 'xfail files', and 'xfail methods'
+ are the possible list heading values:
+
+ skip files
+
+
+
+ xfail methods
+
+"""
+excl_type = None
+case_type = None
+
+with open(exclusion_file) as f:
+for line in f:
+if not excl_type:
+[excl_type, case_type] = line.split()
+continue
+
+line = line.strip()
+if not line:
+excl_type = None
+elif excl_type == 'skip' and case_type == 'files':
+if not configuration.skip_files:
+configuration.skip_files = []
+configuration.skip_files.append(line)
+elif excl_type == 'skip' and case_type == 'methods':
+if not configuration.skip_methods:
+configuration.skip_methods = []
+configuration.skip_methods.append(line)
+elif excl_type == 'xfail'
Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
labath added a comment. I don't think this is a totally bad idea. In fact we already had something like this (nobody used it though), before it was removed in https://reviews.llvm.org/rL255040. If it goes in, we might start using it actually -- e.g., currently we have watchpoint tests which fail on some devices which do not support watchpoints. There is no reasonable thing we can base the expectation as the exact same device with a different cpu revision could support watchpoints just fine, so we could just define the list of these tests externally (in this case, I would probably annotate them with the `watchpoint` category and then do the skips based on categories instead). That said, I do have slightly mixed feelings about it, as it is increasing the complexity of an already complex system, and there are other possible ways to solve the watchpoint problem (have the tests detect whether the device supports watchpoints, and self-skip when appropriate). Comment at: packages/Python/lldbsuite/test/dotest.py:803 @@ +802,3 @@ +if configuration.skip_files: +import re +for file_regexp in configuration.skip_files: We should just `import re` at top level. A lot of tests already do that, so it's not likely it will break anyone. https://reviews.llvm.org/D24629 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
fjricci added a comment. The issue is that you can only commit a patch to xfail a test that fails when you run the test suite on master with no local changes. The problem is that if you run into test failures on other branches or in unconventional configurations, there is no good way to disable failing tests, other than carrying local patches to xfail the tests which fail. Carrying these sorts of local patches is tedious, prone to breakages, and requires many manual changes whenever test suite sources changes. I'm particular, we run into this with ds2, since it fails some tests passed by lldb-server (and passes some tests xfail-ed by lldb-server). I also find that I fail different tests on master (with lldb-server) between Ubuntu and CentOS, for example, and I'm not sure that it makes sense to xfail in those cases. https://reviews.llvm.org/D24629 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24629: Allow for tests to be disabled at runtime
zturner added a comment. If a set of tests is failing, wouldn't you just want to xfail them? https://reviews.llvm.org/D24629 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
