This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8938

run-tests: refactor filtering logic for --retest flag
ClosedPublic

Authored by khanchi97 on Aug 22 2020, 7:05 AM.

Details

Reviewers
marmoute
pulkit
Group Reviewers
hg-reviewers
Commits
rHGde9ec12ee53c: run-tests: refactor filtering logic for --retest flag
Summary

How I got to this:
While re-running failed tests using --retest I noticed that the output:
"running x tests using y parallel processes".
was not actually correct, because x was the total number of tests present
in the directory, but it should be the number of failed tests. Although
it would run only the failed tests and later will say that remaining tests
were skipped.
Changes in test files reflect the fixed behaviour.

This patch change and move the logic for filtering failed test for
--retest option and make sure that we create instances of
class Test only for the tests we need to run.

As mentioned in the deleted text (in this patch itself) the logic
for --retest should be outside of TestSuite.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

khanchi97 created this revision.Aug 22 2020, 7:05 AM
pulkit added a subscriber: pulkit.Aug 24 2020, 3:40 AM
pulkit added inline comments.
tests/run-tests.py
3249

Instead of re-implementing this test list building logic, I think we can just filter the existing tests list which we have above to check for errpath.

khanchi97 added inline comments.Aug 24 2020, 4:20 AM
tests/run-tests.py
3249

We are not re-implementing the test build logic here. If you see next line, I think we are doing what you are suggesting i.e. filtering the existing tests list.

marmoute requested changes to this revision.Aug 26 2020, 12:45 PM
marmoute added a subscriber: marmoute.
marmoute added inline comments.
tests/run-tests.py
3252

I and confuse about this line. What are we joining? Do we have any test for this case?

This revision now requires changes to proceed.Aug 26 2020, 12:45 PM
khanchi97 added inline comments.Aug 26 2020, 1:25 PM
tests/run-tests.py
3252

This is to make test name for "multiple dimensions" testcases (not much familiar with multiple dimensions though). See https://www.mercurial-scm.org/repo/hg-committed/file/tip/tests/test-run-tests.t#l959

Maybe I should consider adding a comment before this line?

marmoute added inline comments.Aug 26 2020, 2:05 PM
tests/run-tests.py
3252

I missed the fact we could have multiple "cases". You should probably document it.

And add a tes for this case with --retest

pulkit accepted this revision.Sep 2 2020, 12:29 PM
pulkit added inline comments.
tests/run-tests.py
3249

Okay, seems like I misread.

Can you follow up by adding a utility function to get errpath since we now have the same logic at couple of places (other one is ~30 lines below).

khanchi97 added inline comments.Sep 2 2020, 1:12 PM
tests/run-tests.py
3249

Sure.

pulkit accepted this revision.Sep 3 2020, 2:15 AM
khanchi97 added a commit: rHGde9ec12ee53c: run-tests: refactor filtering logic for --retest flag.Sep 3 2020, 3:07 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGde9ec12ee53c: run-tests: refactor filtering logic for --retest flag (authored by khanchi97). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mtests/run-tests.py (26 lines)
Mtests/test-run-tests.t (12 lines)

Diff 22526

tests/run-tests.py

"""Custom unittest TestSuite that knows how to execute Mercurial tests.""" """Custom unittest TestSuite that knows how to execute Mercurial tests."""
def __init__( def __init__(
self, self,
testdir, testdir,
jobs=1, jobs=1,
whitelist=None, whitelist=None,
blacklist=None, blacklist=None,
retest=False,
keywords=None, keywords=None,
loop=False, loop=False,
runs_per_test=1, runs_per_test=1,
loadtest=None, loadtest=None,
showchannels=False, showchannels=False,
*args, *args,
**kwargs **kwargs
): ):
whitelist and blacklist denote tests that have been whitelisted and whitelist and blacklist denote tests that have been whitelisted and
blacklisted, respectively. These arguments don't belong in TestSuite. blacklisted, respectively. These arguments don't belong in TestSuite.
Instead, whitelist and blacklist should be handled by the thing that Instead, whitelist and blacklist should be handled by the thing that
populates the TestSuite with tests. They are present to preserve populates the TestSuite with tests. They are present to preserve
backwards compatible behavior which reports skipped tests as part backwards compatible behavior which reports skipped tests as part
of the results. of the results.
retest denotes whether to retest failed tests. This arguably belongs
outside of TestSuite.
keywords denotes key words that will be used to filter which tests keywords denotes key words that will be used to filter which tests
to execute. This arguably belongs outside of TestSuite. to execute. This arguably belongs outside of TestSuite.
loop denotes whether to loop over tests forever. loop denotes whether to loop over tests forever.
""" """
super(TestSuite, self).__init__(*args, **kwargs) super(TestSuite, self).__init__(*args, **kwargs)
self._jobs = jobs self._jobs = jobs
self._whitelist = whitelist self._whitelist = whitelist
self._blacklist = blacklist self._blacklist = blacklist
self._retest = retest
self._keywords = keywords self._keywords = keywords
self._loop = loop self._loop = loop
self._runs_per_test = runs_per_test self._runs_per_test = runs_per_test
self._loadtest = loadtest self._loadtest = loadtest
self._showchannels = showchannels self._showchannels = showchannels
def run(self, result): def run(self, result):
# We have a number of filters that need to be applied. We do this # We have a number of filters that need to be applied. We do this
result.addSkip(test, "Doesn't exist") result.addSkip(test, "Doesn't exist")
continue continue
if not (self._whitelist and test.bname in self._whitelist): if not (self._whitelist and test.bname in self._whitelist):
if self._blacklist and test.bname in self._blacklist: if self._blacklist and test.bname in self._blacklist:
result.addSkip(test, 'blacklisted') result.addSkip(test, 'blacklisted')
continue continue
if self._retest and not os.path.exists(test.errpath):
result.addIgnore(test, 'not retesting')
continue
if self._keywords: if self._keywords:
with open(test.path, 'rb') as f: with open(test.path, 'rb') as f:
t = f.read().lower() + test.bname.lower() t = f.read().lower() + test.bname.lower()
ignored = False ignored = False
for k in self._keywords.lower().split(): for k in self._keywords.lower().split():
if k not in t: if k not in t:
result.addIgnore(test, "doesn't match keyword") result.addIgnore(test, "doesn't match keyword")
ignored = True ignored = True
cases = [] cases = []
else: else:
pass pass
tests += [{'path': t, 'case': c} for c in sorted(cases)] tests += [{'path': t, 'case': c} for c in sorted(cases)]
else: else:
tests.append({'path': t}) tests.append({'path': t})
else: else:
tests.append({'path': t}) tests.append({'path': t})
if self.options.retest:
retest_args = []
pulkitUnsubmitted
Not Done

Instead of re-implementing this test list building logic, I think we can just filter the existing tests list which we have above to check for errpath.

pulkit: Instead of re-implementing this test list building logic, I think we can just filter the…
khanchi97AuthorUnsubmitted
Done

We are not re-implementing the test build logic here. If you see next line, I think we are doing what you are suggesting i.e. filtering the existing tests list.

khanchi97: We are not re-implementing the test build logic here. If you see next line, I think we are…
pulkitUnsubmitted
Not Done

Okay, seems like I misread.

Can you follow up by adding a utility function to get errpath since we now have the same logic at couple of places (other one is ~30 lines below).

pulkit: Okay, seems like I misread. Can you follow up by adding a utility function to get errpath…
khanchi97AuthorUnsubmitted
Done

Sure.

khanchi97: Sure.
for test in tests:
if 'case' in test:
# for multiple dimensions test cases
marmouteUnsubmitted
Not Done

I and confuse about this line. What are we joining? Do we have any test for this case?

marmoute: I and confuse about this line. What are we joining? Do we have any test for this case?
khanchi97AuthorUnsubmitted
Done

This is to make test name for "multiple dimensions" testcases (not much familiar with multiple dimensions though). See https://www.mercurial-scm.org/repo/hg-committed/file/tip/tests/test-run-tests.t#l959

Maybe I should consider adding a comment before this line?

khanchi97: This is to make test name for "multiple dimensions" testcases (not much familiar with multiple…
marmouteUnsubmitted
Not Done

I missed the fact we could have multiple "cases". You should probably document it.

And add a tes for this case with --retest

marmoute: I missed the fact we could have multiple "cases". You should probably document it. And add a…
casestr = b'#'.join(test['case'])
errpath = b'%s#%s.err' % (test['path'], casestr)
else:
errpath = b'%s.err' % test['path']
if self.options.outputdir:
errpath = os.path.join(self.options.outputdir, errpath)
if os.path.exists(errpath):
retest_args.append(test)
tests = retest_args
return tests return tests
def _runtests(self, testdescs): def _runtests(self, testdescs):
def _reloadtest(test, i): def _reloadtest(test, i):
# convert a test back to its description dict # convert a test back to its description dict
desc = {'path': test.path} desc = {'path': test.path}
case = getattr(test, '_case', []) case = getattr(test, '_case', [])
if case: if case:
if kws is not None and PYTHON3: if kws is not None and PYTHON3:
kws = kws.encode('utf-8') kws = kws.encode('utf-8')
suite = TestSuite( suite = TestSuite(
self._testdir, self._testdir,
jobs=jobs, jobs=jobs,
whitelist=self.options.whitelisted, whitelist=self.options.whitelisted,
blacklist=self.options.blacklist, blacklist=self.options.blacklist,
retest=self.options.retest,
keywords=kws, keywords=kws,
loop=self.options.loop, loop=self.options.loop,
runs_per_test=self.options.runs_per_test, runs_per_test=self.options.runs_per_test,
showchannels=self.options.showchannels, showchannels=self.options.showchannels,
tests=tests, tests=tests,
loadtest=_reloadtest, loadtest=_reloadtest,
) )
verbosity = 1 verbosity = 1

tests/test-run-tests.t

</testsuite> </testsuite>
$ rm test-failure-unicode.t $ rm test-failure-unicode.t
test for --retest test for --retest
==================== ====================
$ rt --retest $ rt --retest
running 2 tests using 1 parallel processes running 1 tests using 1 parallel processes
--- $TESTTMP/test-failure.t --- $TESTTMP/test-failure.t
+++ $TESTTMP/test-failure.t.err +++ $TESTTMP/test-failure.t.err
@@ -1,5 +1,5 @@ @@ -1,5 +1,5 @@
$ echo babar $ echo babar
- rataxes - rataxes
+ babar + babar
This is a noop statement so that This is a noop statement so that
this test is still more bytes than success. this test is still more bytes than success.
pad pad pad pad............................................................ pad pad pad pad............................................................
ERROR: test-failure.t output changed ERROR: test-failure.t output changed
! !
Failed test-failure.t: output changed Failed test-failure.t: output changed
# Ran 2 tests, 1 skipped, 1 failed. # Ran 1 tests, 0 skipped, 1 failed.
python hash seed: * (glob) python hash seed: * (glob)
[1] [1]
--retest works with --outputdir --retest works with --outputdir
$ rm -r output $ rm -r output
$ mkdir output $ mkdir output
$ mv test-failure.t.err output $ mv test-failure.t.err output
$ rt --retest --outputdir output $ rt --retest --outputdir output
running 2 tests using 1 parallel processes running 1 tests using 1 parallel processes
--- $TESTTMP/test-failure.t --- $TESTTMP/test-failure.t
+++ $TESTTMP/output/test-failure.t.err +++ $TESTTMP/output/test-failure.t.err
@@ -1,5 +1,5 @@ @@ -1,5 +1,5 @@
$ echo babar $ echo babar
- rataxes - rataxes
+ babar + babar
This is a noop statement so that This is a noop statement so that
this test is still more bytes than success. this test is still more bytes than success.
pad pad pad pad............................................................ pad pad pad pad............................................................
ERROR: test-failure.t output changed ERROR: test-failure.t output changed
! !
Failed test-failure.t: output changed Failed test-failure.t: output changed
# Ran 2 tests, 1 skipped, 1 failed. # Ran 1 tests, 0 skipped, 1 failed.
python hash seed: * (glob) python hash seed: * (glob)
[1] [1]
Selecting Tests To Run Selecting Tests To Run
====================== ======================
successful successful
ERROR: test-cases.t#b#c output changed ERROR: test-cases.t#b#c output changed
!. !.
Failed test-cases.t#b#c: output changed Failed test-cases.t#b#c: output changed
# Ran 4 tests, 0 skipped, 1 failed. # Ran 4 tests, 0 skipped, 1 failed.
python hash seed: * (glob) python hash seed: * (glob)
[1] [1]
$ rt --retest $ rt --retest
running 5 tests using 1 parallel processes running 1 tests using 1 parallel processes
--- $TESTTMP/test-cases.t --- $TESTTMP/test-cases.t
+++ $TESTTMP/test-cases.t#b#c.err +++ $TESTTMP/test-cases.t#b#c.err
@@ -6,5 +6,5 @@ @@ -6,5 +6,5 @@
#endif #endif
#if b c #if b c
$ echo yes $ echo yes
- no - no
+ yes + yes
#endif #endif
ERROR: test-cases.t#b#c output changed ERROR: test-cases.t#b#c output changed
! !
Failed test-cases.t#b#c: output changed Failed test-cases.t#b#c: output changed
# Ran 5 tests, 4 skipped, 1 failed. # Ran 1 tests, 0 skipped, 1 failed.
python hash seed: * (glob) python hash seed: * (glob)
[1] [1]
$ rm test-cases.t#b#c.err $ rm test-cases.t#b#c.err
$ rm test-cases.t $ rm test-cases.t
(reinstall) (reinstall)
$ mv backup test-failure.t $ mv backup test-failure.t