Page MenuHomePhabricator

testrunner: allow multiple #testcases
ClosedPublic

Authored by martinvonz on Aug 1 2018, 7:52 PM.

Details

Summary

This lets you have multiple #testcases declarations and they're taken
to be different dimensions of the test, so their cross product becomes
the total set of test cases. For example:

#testcases obsstore-on obsstore-off
#testcases manifest-flat manifest-tree

  $ hg rebase ...
  ...
  #if obsstore-on
    $ hg log ...
  #endif

Note that this is an excellent way to slow down the test suite, so use
it with care.

The feature is implemented by replacing most of the "case" variables
that were strings before by an array of strings with each item a
different dimension of the test case. The file names are created by
joining the dimensions by "#"
(e.g. test-foo.t#obsstore-on#manifest-flat).

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

martinvonz created this revision.Aug 1 2018, 7:52 PM
mharbison72 requested changes to this revision.Aug 2 2018, 1:07 AM
mharbison72 added a subscriber: mharbison72.

I think Windows will choke on ‘:’ in the file name.

This revision now requires changes to proceed.Aug 2 2018, 1:07 AM

I think Windows will choke on ‘:’ in the file name.

What's a better character for Windows?

Not sure. If we can’t reuse ‘#’, maybe ‘_’ would be ok as a distinct visual identifier? I don’t think either are reserved characters.

Not sure. If we can’t reuse ‘#’, maybe ‘_’ would be ok as a distinct visual identifier? I don’t think either are reserved characters.

I was thinking of using '#'. It would be kind of nice because we already use that for specifying test cases from the command line (e.g. run-tests.py test-foo.t#obsmarkers-on). It would seem consistent with that to use '#' before each dimension. However, in the output files (e.g. '.err'), we use a '.' as separator. I don't know if Windows is the reason for that. A quick internet search suggested it isn't. I'll switch to that and hopefully you or someone else can tell me if it seems to work.

martinvonz edited the summary of this revision. (Show Details)Aug 2 2018, 1:38 AM
martinvonz updated this revision to Diff 9784.

Not sure. If we can’t reuse ‘#’, maybe ‘_’ would be ok as a distinct visual identifier? I don’t think either are reserved characters.

I was thinking of using '#'. It would be kind of nice because we already use that for specifying test cases from the command line (e.g. run-tests.py test-foo.t#obsmarkers-on). It would seem consistent with that to use '#' before each dimension. However, in the output files (e.g. '.err'), we use a '.' as separator. I don't know if Windows is the reason for that. A quick internet search suggested it isn't. I'll switch to that and hopefully you or someone else can tell me if it seems to work.

I inserted a parent (D4052) that changes the format of .err files and changed the separator here to #. I like this much better. Hopefully it works on Windows.

lothiraldan added inline comments.
tests/test-run-tests.t
912

Could you add a combination that failed so we test the test name of combination cases?

martinvonz marked an inline comment as done.Aug 2 2018, 11:00 AM
martinvonz updated this revision to Diff 9790.

Added a failed test

I'm getting an import error, and I'm not sure how to figure out the parent revision from phab.

$ hg phabimport D4049
applying patch from stdin
patching file tests/run-tests.py
Hunk #2 FAILED at 1241
Hunk #7 FAILED at 2724
abort: patch failed to apply

$ hg config alias.phabimport
!"%HG%" phabread $1 --stack | "%HG%" import --bypass -

I'm getting an import error, and I'm not sure how to figure out the parent revision from phab.

Make sure you're applying it onto @ from https://www.mercurial-scm.org/repo/hg-committed/. If not, you'll probably have to import D4052 before this patch (D4052 is in @ on hg-committed).

I manually fixed it up, and got the following diffs. I see the last parent is listed as D4067, which hasn't been accepted yet, so I'm assuming that's the issue (I'm way behind on the ML). And I guess --stack doesn't pick up parent dependencies, just what was submitted together?

--- c:/Users/Matt/projects/hg/tests/test-run-tests.t
+++ c:/Users/Matt/projects/hg/tests/test-run-tests.t.err
@@ -850,7 +850,7 @@
   > EOF

   --- $TESTTMP/test-cases.t
-  +++ $TESTTMP/test-cases.t.a.err
+  +++ $TESTTMP/test-cases.t#a.err
   @@ -1,6 +1,7 @@
    #testcases a b
    #if a
@@ -861,7 +861,7 @@
      $ echo 2
   Accept this change? [n] .
   --- $TESTTMP/test-cases.t
-  +++ $TESTTMP/test-cases.t.b.err
+  +++ $TESTTMP/test-cases.t#b.err
   @@ -5,4 +5,5 @@
    #endif
    #if b
@@ -1574,7 +1574,7 @@
   $ rt
   .
   --- $TESTTMP/anothertests/cases/test-cases-abc.t
-  +++ $TESTTMP/anothertests/cases/test-cases-abc.t.B.err
+  +++ $TESTTMP/anothertests/cases/test-cases-abc.t#B.err
   @@ -7,7 +7,7 @@
      $ V=C
    #endif
@@ -1597,7 +1597,7 @@
   $ rt --restart

   --- $TESTTMP/anothertests/cases/test-cases-abc.t
-  +++ $TESTTMP/anothertests/cases/test-cases-abc.t.B.err
+  +++ $TESTTMP/anothertests/cases/test-cases-abc.t#B.err
   @@ -7,7 +7,7 @@
      $ V=C
    #endif
@@ -1619,10 +1619,12 @@

   $ mkdir output
   $ mv test-cases-abc.t.B.err output
+  mv: cannot stat `test-cases-abc.t.B.err': $ENOENT$
+  [1]
   $ rt --restart --outputdir output
-
+  .
   --- $TESTTMP/anothertests/cases/test-cases-abc.t
-  +++ $TESTTMP/anothertests/cases/output/test-cases-abc.t.B.err
+  +++ $TESTTMP/anothertests/cases/output/test-cases-abc.t#B.err
   @@ -7,7 +7,7 @@
      $ V=C
    #endif
@@ -1636,8 +1638,9 @@
   ERROR: test-cases-abc.t#B output changed
   !.
   Failed test-cases-abc.t#B: output changed
-  # Ran 2 tests, 0 skipped, 1 failed.
-  python hash seed: * (glob)
+  # Ran 3 tests, 0 skipped, 1 failed.
+  python hash seed: * (glob)
+  running all tests
   [1]

 Test TESTCASE variable
@@ -1665,7 +1668,7 @@
   $ rt "test-cases-abc.t#B"

   --- $TESTTMP/anothertests/cases/test-cases-abc.t
-  +++ $TESTTMP/anothertests/cases/test-cases-abc.t.B.err
+  +++ $TESTTMP/anothertests/cases/test-cases-abc.t#B.err
   @@ -7,7 +7,7 @@
      $ V=C
    #endif
@@ -1688,7 +1691,7 @@
   $ rt test-cases-abc.t#B test-cases-abc.t#C

   --- $TESTTMP/anothertests/cases/test-cases-abc.t
-  +++ $TESTTMP/anothertests/cases/test-cases-abc.t.B.err
+  +++ $TESTTMP/anothertests/cases/test-cases-abc.t#B.err
   @@ -7,7 +7,7 @@
      $ V=C
    #endif
@@ -1711,7 +1714,7 @@
   $ rt test-cases-abc.t#B test-cases-abc.t#D

   --- $TESTTMP/anothertests/cases/test-cases-abc.t
-  +++ $TESTTMP/anothertests/cases/test-cases-abc.t.B.err
+  +++ $TESTTMP/anothertests/cases/test-cases-abc.t#B.err
   @@ -7,7 +7,7 @@
      $ V=C
    #endif
@@ -1745,7 +1748,7 @@
   $ rt test-cases-advanced-cases.t

   --- $TESTTMP/anothertests/cases/test-cases-advanced-cases.t
-  +++ $TESTTMP/anothertests/cases/test-cases-advanced-cases.t.case-with-dashes.err
+  +++ $TESTTMP/anothertests/cases/test-cases-advanced-cases.t#case-with-dashes.err
   @@ -1,3 +1,3 @@
    #testcases simple case-with-dashes casewith_-.chars
      $ echo $TESTCASE
@@ -1755,7 +1758,7 @@
   ERROR: test-cases-advanced-cases.t#case-with-dashes output changed
   !
   --- $TESTTMP/anothertests/cases/test-cases-advanced-cases.t
-  +++ $TESTTMP/anothertests/cases/test-cases-advanced-cases.t.casewith_-.chars.err
+  +++ $TESTTMP/anothertests/cases/test-cases-advanced-cases.t#casewith_-.chars.err
   @@ -1,3 +1,3 @@
    #testcases simple case-with-dashes casewith_-.chars
      $ echo $TESTCASE
@@ -1773,7 +1776,7 @@
   $ rt "test-cases-advanced-cases.t#case-with-dashes"

   --- $TESTTMP/anothertests/cases/test-cases-advanced-cases.t
-  +++ $TESTTMP/anothertests/cases/test-cases-advanced-cases.t.case-with-dashes.err
+  +++ $TESTTMP/anothertests/cases/test-cases-advanced-cases.t#case-with-dashes.err
   @@ -1,3 +1,3 @@
    #testcases simple case-with-dashes casewith_-.chars
      $ echo $TESTCASE
@@ -1790,7 +1793,7 @@
   $ rt "test-cases-advanced-cases.t#casewith_-.chars"

   --- $TESTTMP/anothertests/cases/test-cases-advanced-cases.t
-  +++ $TESTTMP/anothertests/cases/test-cases-advanced-cases.t.casewith_-.chars.err
+  +++ $TESTTMP/anothertests/cases/test-cases-advanced-cases.t#casewith_-.chars.err
   @@ -1,3 +1,3 @@
    #testcases simple case-with-dashes casewith_-.chars
      $ echo $TESTCASE

ERROR: test-run-tests.t output changed
!

I'm getting an import error, and I'm not sure how to figure out the parent revision from phab.

Make sure you're applying it onto @ from https://www.mercurial-scm.org/repo/hg-committed/. If not, you'll probably have to import D4052 before this patch (D4052 is in @ on hg-committed).

Maybe needs a push? I'm seeing @ on D4048 on hg-committed.

I'm getting an import error, and I'm not sure how to figure out the parent revision from phab.

Make sure you're applying it onto @ from https://www.mercurial-scm.org/repo/hg-committed/. If not, you'll probably have to import D4052 before this patch (D4052 is in @ on hg-committed).

Maybe needs a push? I'm seeing @ on D4048 on hg-committed.

Sorry, I was just misremembering that D4052 had been queued. I don't know why phabricator doesn't understand that this patch is on top of D4052. Anyway, try applying D4052 first, then this one.

quark added a subscriber: quark.Aug 3 2018, 2:00 AM

Sorry, I was just misremembering that D4052 had been queued. I don't know why phabricator doesn't understand that this patch is on top of D4052. Anyway, try applying D4052 first, then this one.

There is no formal API to set dependency. So you need to click "Edit Related Revisions... -> Edit Parent Revisions" on the right sidebar manually.

mharbison72 accepted this revision.Aug 3 2018, 2:01 AM

I'm getting an import error, and I'm not sure how to figure out the parent revision from phab.

Make sure you're applying it onto @ from https://www.mercurial-scm.org/repo/hg-committed/. If not, you'll probably have to import D4052 before this patch (D4052 is in @ on hg-committed).

Maybe needs a push? I'm seeing @ on D4048 on hg-committed.

Sorry, I was just misremembering that D4052 had been queued. I don't know why phabricator doesn't understand that this patch is on top of D4052. Anyway, try applying D4052 first, then this one.

That did the trick, and this runs clean on Windows, thanks.

It seems to add ~4 seconds to test-run-tests.t, but that's probably not a big deal if it doesn't affect all of the other tests like this.

I'm getting an import error, and I'm not sure how to figure out the parent revision from phab.

Make sure you're applying it onto @ from https://www.mercurial-scm.org/repo/hg-committed/. If not, you'll probably have to import D4052 before this patch (D4052 is in @ on hg-committed).

Maybe needs a push? I'm seeing @ on D4048 on hg-committed.

Sorry, I was just misremembering that D4052 had been queued. I don't know why phabricator doesn't understand that this patch is on top of D4052. Anyway, try applying D4052 first, then this one.

That did the trick, and this runs clean on Windows, thanks.

Great. Thanks for checking!

It seems to add ~4 seconds to test-run-tests.t, but that's probably not a big deal if it doesn't affect all of the other tests like this.

That's probably just because it adds a recursive call to run 4 tests. I doubt it has any measurable impact on other tests.

In D4049#63027, @quark wrote:

Sorry, I was just misremembering that D4052 had been queued. I don't know why phabricator doesn't understand that this patch is on top of D4052. Anyway, try applying D4052 first, then this one.

There is no formal API to set dependency. So you need to click "Edit Related Revisions... -> Edit Parent Revisions" on the right sidebar manually.

Thanks! I had never noticed that sidebar.

In D4049#63027, @quark wrote:

Sorry, I was just misremembering that D4052 had been queued. I don't know why phabricator doesn't understand that this patch is on top of D4052. Anyway, try applying D4052 first, then this one.

There is no formal API to set dependency. So you need to click "Edit Related Revisions... -> Edit Parent Revisions" on the right sidebar manually.

That's the closest thing I found to determining what the parent was. Even if there's no formal API to set that dependency, is there an API to read the parent? I'm wondering if an --exact switch is possible on phabread, similar to --stack, but pull in all parents recursively.

quark added a comment.Aug 3 2018, 2:16 AM

--stack should work as expected if dependency is set manually.

pulkit added a subscriber: pulkit.Aug 3 2018, 9:59 AM

Thanks to @mharbison72 for review and testing it on windows. Queuing this, many thanks!

pulkit accepted this revision.Aug 3 2018, 10:00 AM
pulkit added inline comments.Aug 3 2018, 10:06 AM
tests/run-tests.py
2705–2706

Fixed this bad indent in flight.

This revision was automatically updated to reflect the committed changes.