This is an archive of the discontinued Mercurial Phabricator instance.

Differential D950

run-tests: update back to original node after bisecting
AbandonedPublic

Authored by quark on Oct 5 2017, 12:22 AM.

Details

Reviewers
ryanmce
Group Reviewers
hg-reviewers
Summary

The bisect operation should have minimal side-effect.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Oct 5 2017, 12:22 AM
ryanmce requested changes to this revision.Oct 12 2017, 7:34 AM
ryanmce added a subscriber: ryanmce.

I'm +1 on this series and the rest looks fine but I think this one needs some improvements before landing.

tests/run-tests.py
2114–2115

It feels fragile to rely on the output of a debug flag command to determine state. Can you use hg status instead?

2115–2145

For readability, please add the try/finally (with the finally block being just pass for now) in a separate patch.

2119–2120

When does this happen? Add a comment, perhaps? I'd also prefer a more explanatory output.

This revision now requires changes to proceed.Oct 12 2017, 7:34 AM
quark added inline comments.Oct 12 2017, 11:34 AM
tests/run-tests.py
2115–2145

During the meeting deciding to experiment with Phabricator, I think we agreed that with Phabricator's ability to diff with whitespaces ignored and its yellow margins of "Moved from line X", the patch size standard could be more flexible.

quark updated this revision to Diff 2649.Oct 12 2017, 8:11 PM
ryanmce added inline comments.Oct 13 2017, 10:51 AM
tests/run-tests.py
2115–2145

I suppose, I ended up using that ability but it would still help readability either way. I won't fuss too much over it.

ryanmce accepted this revision as: ryanmce.Oct 13 2017, 11:08 AM

I like this overall; but I don't feel I can queue a new flag to bisect without input from the rest of the community.

quark abandoned this revision.Oct 17 2017, 1:00 AM

Revision Contents
Changeset List

PathPackages
Mtests/run-tests.py (55 lines)
Mtests/test-run-tests.t (8 lines)

Diff 2649

tests/run-tests.py

def pread(args): def pread(args):
env = os.environ.copy() env = os.environ.copy()
env['HGPLAIN'] = '1' env['HGPLAIN'] = '1'
p = subprocess.Popen(args, stderr=subprocess.STDOUT, p = subprocess.Popen(args, stderr=subprocess.STDOUT,
stdout=subprocess.PIPE, env=env) stdout=subprocess.PIPE, env=env)
data = p.stdout.read() data = p.stdout.read()
p.wait() p.wait()
return data return data
for test in tests: for test in tests:
try:
ryanmceUnsubmitted
Not Done

It feels fragile to rely on the output of a debug flag command to determine state. Can you use hg status instead?

ryanmce: It feels fragile to rely on the output of a debug flag command to determine state. Can you use…
pread(bisectcmd + ['--reset']), pread(bisectcmd + ['--reset']),
pread(bisectcmd + ['--bad', '.']) pread(bisectcmd + ['--bad', '.'])
pread(bisectcmd + ['--good', pread(bisectcmd + ['--good',
self._runner.options.known_good_rev]) self._runner.options.known_good_rev])
# TODO: we probably need to forward more options # TODO: we probably need to forward more options
ryanmceUnsubmitted
Not Done

When does this happen? Add a comment, perhaps? I'd also prefer a more explanatory output.

ryanmce: When does this happen? Add a comment, perhaps? I'd also prefer a more explanatory output.
# that alter hg's behavior inside the tests. # that alter hg's behavior inside the tests.
opts = '' opts = ''
withhg = self._runner.options.with_hg withhg = self._runner.options.with_hg
if withhg: if withhg:
opts += ' --with-hg=%s ' % shellquote(_strpath(withhg)) opts += ' --with-hg=%s ' % shellquote(_strpath(withhg))
rtc = '%s %s %s %s' % (sys.executable, sys.argv[0], opts, rtc = '%s %s %s %s' % (sys.executable, sys.argv[0], opts,
test) test)
data = pread(bisectcmd + ['--command', rtc]) data = pread(bisectcmd + ['--command', rtc])
m = re.search( m = re.search(
(br'\nThe first (?P<goodbad>bad|good) revision ' (br'\nThe first (?P<goodbad>bad|good) revision '
br'is:\nchangeset: +\d+:(?P<node>[a-f0-9]+)\n.*\n' br'is:\nchangeset: +\d+:(?P<node>[a-f0-9]+)\n.*\n'
br'summary: +(?P<summary>[^\n]+)\n'), br'summary: +(?P<summary>[^\n]+)\n'),
data, (re.MULTILINE | re.DOTALL)) data, (re.MULTILINE | re.DOTALL))
if m is None: if m is None:
self.stream.writeln( self.stream.writeln(
'Failed to identify failure point for %s' % test) 'Failed to identify failure point for %s' % test)
continue continue
dat = m.groupdict() dat = m.groupdict()
verb = 'broken' if dat['goodbad'] == 'bad' else 'fixed' verb = 'broken' if dat['goodbad'] == 'bad' else 'fixed'
self.stream.writeln( self.stream.writeln(
'%s %s by %s (%s)' % ( '%s %s by %s (%s)' % (
test, verb, dat['node'], dat['summary'])) test, verb, dat['node'], dat['summary']))
finally:
pread(bisectcmd + ['--abort'])
ryanmceUnsubmitted
Not Done

For readability, please add the try/finally (with the finally block being just pass for now) in a separate patch.

ryanmce: For readability, please add the try/finally (with the finally block being just `pass` for now)…
quarkAuthorUnsubmitted
Not Done

During the meeting deciding to experiment with Phabricator, I think we agreed that with Phabricator's ability to diff with whitespaces ignored and its yellow margins of "Moved from line X", the patch size standard could be more flexible.

quark: During the meeting deciding to experiment with Phabricator, I think we agreed that with…
ryanmceUnsubmitted
Not Done

I suppose, I ended up using that ability but it would still help readability either way. I won't fuss too much over it.

ryanmce: I suppose, I ended up using that ability but it would still help readability either way. I…
def printtimes(self, times): def printtimes(self, times):
# iolock held by run # iolock held by run
self.stream.writeln('# Producing time report') self.stream.writeln('# Producing time report')
times.sort(key=lambda t: (t[3])) times.sort(key=lambda t: (t[3]))
cols = '%7.3f %7.3f %7.3f %7.3f %7.3f %s' cols = '%7.3f %7.3f %7.3f %7.3f %7.3f %s'
self.stream.writeln('%-7s %-7s %-7s %-7s %-7s %s' % self.stream.writeln('%-7s %-7s %-7s %-7s %-7s %s' %
('start', 'end', 'cuser', 'csys', 'real', 'Test')) ('start', 'end', 'cuser', 'csys', 'real', 'Test'))
for tdata in times: for tdata in times:

tests/test-run-tests.t

> EOF > EOF
$ hg add test-bisect.t $ hg add test-bisect.t
$ hg ci -m 'good' $ hg ci -m 'good'
$ cat >> test-bisect.t <<EOF $ cat >> test-bisect.t <<EOF
> $ echo pass > $ echo pass
> fail > fail
> EOF > EOF
$ hg ci -m 'bad' $ hg ci -m 'bad'
$ echo 'unrelated' > unrelated
$ hg ci -m 'unrelated' -A unrelated
$ rt --known-good-rev=0 test-bisect.t $ rt --known-good-rev=0 test-bisect.t
--- $TESTTMP/anothertests/bisect/test-bisect.t --- $TESTTMP/anothertests/bisect/test-bisect.t
+++ $TESTTMP/anothertests/bisect/test-bisect.t.err +++ $TESTTMP/anothertests/bisect/test-bisect.t.err
@@ -1,4 +1,4 @@ @@ -1,4 +1,4 @@
$ echo pass $ echo pass
pass pass
$ echo pass $ echo pass
- fail - fail
+ pass + pass
ERROR: test-bisect.t output changed ERROR: test-bisect.t output changed
! !
Failed test-bisect.t: output changed Failed test-bisect.t: output changed
test-bisect.t broken by 72cbf122d116 (bad) test-bisect.t broken by 72cbf122d116 (bad)
# Ran 1 tests, 0 skipped, 1 failed. # Ran 1 tests, 0 skipped, 1 failed.
python hash seed: * (glob) python hash seed: * (glob)
[1] [1]
$ hg id -R ../bisect
020d1494060d tip
$ cd .. $ cd ..
support bisecting a separate repo support bisecting a separate repo
$ hg init bisect-dependent $ hg init bisect-dependent
$ cd bisect-dependent $ cd bisect-dependent
$ cat > test-bisect-dependent.t <<EOF $ cat > test-bisect-dependent.t <<EOF
> $ tail -1 \$TESTDIR/../bisect/test-bisect.t > $ tail -1 \$TESTDIR/../bisect/test-bisect.t
ERROR: test-bisect-dependent.t output changed ERROR: test-bisect-dependent.t output changed
! !
Failed test-bisect-dependent.t: output changed Failed test-bisect-dependent.t: output changed
test-bisect-dependent.t broken by 72cbf122d116 (bad) test-bisect-dependent.t broken by 72cbf122d116 (bad)
# Ran 1 tests, 0 skipped, 1 failed. # Ran 1 tests, 0 skipped, 1 failed.
python hash seed: * (glob) python hash seed: * (glob)
[1] [1]
$ hg id -R ../bisect
020d1494060d tip
$ cd .. $ cd ..
Test a broken #if statement doesn't break run-tests threading. Test a broken #if statement doesn't break run-tests threading.
============================================================== ==============================================================
$ mkdir broken $ mkdir broken
$ cd broken $ cd broken
$ cat > test-broken.t <<EOF $ cat > test-broken.t <<EOF
> true > true