1171971 - Mochitest move 'test_paths' out of mach and into MochitestArgumentParser; remove --test-path

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Andrew Halberstadt [:ahal]]

This is actually a somewhat invasive change, as there are a lot of assumptions in the mochitest harness around working with a single test path. I'm going to look into how difficult it would be to remove this assumption.

This will get us 99% of the way to a consolidated command line between mach and runtests.py (the only exception being --flavor). It will also make running mochitests out of a tests.zip more similar to its in-source cousin.

This soft blocks bug 1171602, not required but nice to have.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester

Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8621202 [details]
MozReview Request: Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester

https://reviewboard.mozilla.org/r/10921/#review9561

::: testing/mochitest/runtests.py:2499
(Diff revision 1)
> -        options.testPath = options.testPath.replace("\\", "\\\\")
> +        options.test_paths = [tp.replace("\\", "\\\\") for tp in options.test_paths]

I think this has implications for js harnesses...

::: testing/testsuite-targets.mk:7
(Diff revision 1)
>  ifdef TEST_PATH
> -TEST_PATH_ARG := --test-path='$(TEST_PATH)'
> -IPCPLUGINS_PATH_ARG := --test-path='$(TEST_PATH)'
> +TEST_PATH_ARG := '$(TEST_PATH)'
> +IPCPLUGINS_PATH_ARG := '$(TEST_PATH)'
>  else
>  TEST_PATH_ARG :=
> -IPCPLUGINS_PATH_ARG := --test-path=dom/plugins/test
> +IPCPLUGINS_PATH_ARG := dom/plugins/test
>  endif

I think we can get rid of all this and just put $(TEST_PATH) or dom/plugins/test below.

::: testing/mochitest/browser-harness.xul:106
(Diff revision 1)
>        if (gConfig.testPath)
>          document.getElementById("runTestsButton").label =
>            "Run " + gConfig.testPath + " tests";

This will be gConfig.test_paths now, we probably need to update places it's handled in a few places.

I don't know the extent of it, but there's some code in chrome-harness.js that looks a little scary.

::: testing/mochitest/runtests.py
(Diff revision 1)
> -                self.urlOpts.append("testname=%s" %
> -                                    ("/").join([self.TEST_PATH, options.testPath]))

server.js seems to use this parameter, but I can't tell if it hurts anything to lose it.

Comment 3

[Andrew Halberstadt [:ahal]]

I think my patch actually stops passing in testPath into the JS harness wholesale. So I think that JS stuff is now more or less dead code, but I'll spend some time digging into it a bit. I also realized that the test paths don't get normalized anymore when running from mach, will fix that too.

Btw, here's a full try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33e08063300a

Looks like I only broke Moth on windows.

Comment 4

[(not currently active) Ted Mielczarek]

You'll want to check that you haven't broken the "run a single test from mach" use case, Gijs did a lot of work preserving that when making other changes, it's important to bz and other developers. They expect to be able to run a single test and edit/reload it in the harness without having to restart the test run.

Comment 5

[:Gijs (he/him)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> You'll want to check that you haven't broken the "run a single test from
> mach" use case, Gijs did a lot of work preserving that when making other
> changes, it's important to bz and other developers. They expect to be able
> to run a single test and edit/reload it in the harness without having to
> restart the test run.

FWIW, that code is here:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/mach_commands.py#346

 and recently got an XXX comment, presumably because the background to it was not obvious. Well, there it is ^.


Assuming that code is still correct nothing should be broken here.

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> > You'll want to check that you haven't broken the "run a single test from
> > mach" use case, Gijs did a lot of work preserving that when making other
> > changes, it's important to bz and other developers. They expect to be able
> > to run a single test and edit/reload it in the harness without having to
> > restart the test run.
> 
> FWIW, that code is here:
> 
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/
> mach_commands.py#346
> 
>  and recently got an XXX comment, presumably because the background to it
> was not obvious. Well, there it is ^.
> 
> 
> Assuming that code is still correct nothing should be broken here.

OK, I might have spoken without reading most of the rest of the bug. It's probably worth testing this case explicitly to make sure nothing much else broke this.

Comment 7

[Andrew Halberstadt [:ahal]]

Thanks for the pointer, will do.

p.s. The comment is mostly asking why is this behaviour only enabled for 'plain' as opposed to all suites. If people think it's ok, I can remove the suite == 'plain' check.

Comment 8

[Andrew Halberstadt [:ahal]]

It still works, mach never used --test-path so not passing that into the JS side won't have an affect on any mach commands.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

(In reply to Andrew Halberstadt [:ahal] from comment #3)
> I think my patch actually stops passing in testPath into the JS harness
> wholesale. So I think that JS stuff is now more or less dead code, but I'll
> spend some time digging into it a bit. I also realized that the test paths
> don't get normalized anymore when running from mach, will fix that too.
> 
> Btw, here's a full try run:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=33e08063300a
> 
> Looks like I only broke Moth on windows.

Ok, makes sense. Maybe we should just pass testPath when there's a single path to preserve any behavior for people who care about it (it looks like it might be used to generate the list of tests in the UI, which someone asked about recently), and do whatever would have happened before with the mach command when we have more than one.

Comment 10

[Andrew Halberstadt [:ahal]]

Here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cfc0e6f3ece

The RC5 perma fails show up on try pushes prior to mine.

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8621202 [details]
MozReview Request: Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester

Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8621202 [details]
MozReview Request: Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester

https://reviewboard.mozilla.org/r/10921/#review9775

::: testing/mochitest/browser-test.js:353
(Diff revision 2)
> -                                             "No tests to run. Did you pass an invalid --test-path?");
> +                                             "No tests to run. Did you pass an invalid test_paths?");

nit: "Did you pass invalid test_paths?" and below.

::: testing/mochitest/runtests.py:1926
(Diff revisions 1 - 2)
> +                    if not os.path.exists(runtime_file):
> +                        self.log.warning("runtime file %s not found; defaulting to chunk-by-dir" %
> +                                         runtime_file)
> +                        options.chunkByRuntime = None
> +                        flavor = self.getTestFlavor(options)
> +                        if flavor in ('browser-chrome', 'devtools-chrome'):
> +                            # these values match current mozharness configs
> +                            options.chunkbyDir = 5
> +                        else:
> +                            options.chunkByDir = 4

I haven't been following the chunk by runtime work very closely, but this change seems unrelated.

::: testing/mochitest/runtests.py:2526
(Diff revision 2)
>          "Creates a test configuration file for customizing test execution."
>          options.logFile = options.logFile.replace("\\", "\\\\")
> -        options.testPath = options.testPath.replace("\\", "\\\\")
> +        options.test_paths = [tp.replace("\\", "\\\\") for tp in options.test_paths]

What was the conclusion here? It seems like:

    if len(options.test_paths) == 1:
        options.testPath = options.test_paths[0].replace("\\", "\\\\")
        
would preserve any current behavior. If we're sure this isn't used anywhere we should probably remove all the dead code it's creating on the JS side too.

Looks good overall. I had some issues from last time that aren't fixed or dropped.

Comment 13

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/10921/#review9789

> I haven't been following the chunk by runtime work very closely, but this change seems unrelated.

This is fixing a merge conflict.

> What was the conclusion here? It seems like:
> 
>     if len(options.test_paths) == 1:
>         options.testPath = options.test_paths[0].replace("\\", "\\\\")
>         
> would preserve any current behavior. If we're sure this isn't used anywhere we should probably remove all the dead code it's creating on the JS side too.

It looks like testPath only ever got passed into the JS side if we run a single test and options.repeat > 0. This latest patch still passes it in if len == 1, so previous behaviour should still be preserved.

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 8621202 [details]
MozReview Request: Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester

Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester

Comment 15

[Andrew Halberstadt [:ahal]]

Latest just fixes the nits, but I closed some of your issues, so want to make sure you're ok with it.

To the best of my knowledge, this shouldn't add any dead code on the JS side that didn't exist before. It looks like ?testname=... was only passed in if options.repeat > 0 and testPath was a file. This patch still does that except with the added condition that len(options.test_paths) == 1.

I strongly suspect that the JS harness already has a lot of dead code due to the many previous refactorings the mochitest harness has undergone.

Comment 16

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/10921/#review9635

> I think we can get rid of all this and just put $(TEST_PATH) or dom/plugins/test below.

$(TEST_PATH) isn't always defined, so I think I'd have to make this check in N places instead of 1. Anyway, I'd rather just leave it as is if that's ok.

> This will be gConfig.test_paths now, we probably need to update places it's handled in a few places.
> 
> I don't know the extent of it, but there's some code in chrome-harness.js that looks a little scary.

The url format isn't changed at all, so this should still be testPath.

Comment 17

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/10921/#review9793

> It looks like testPath only ever got passed into the JS side if we run a single test and options.repeat > 0. This latest patch still passes it in if len == 1, so previous behaviour should still be preserved.

A bit below here almost all of options get serialized and passed up to the js harnesses. This gets used in the getTestList function, which has a few callers. I think in the cases we care about we already constructed a manifest with the tests we care about, so maybe it doesn't matter, but knowingly creating a bunch of dead code like this seems wrong.

Comment 18

[Andrew Halberstadt [:ahal]]

Ah, I didn't notice that, thanks.

Comment 19

[Andrew Halberstadt [:ahal]]

Actually looks like that entire getFileListing function has been dead code for a long time because of this:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/chrome-harness.js#316

The python harness always sets options.manifestFile here:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#790

I'll try to remove it and see what happens.

Comment 20

[Andrew Halberstadt [:ahal]]

Comment on attachment 8621202 [details]
MozReview Request: Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester

Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester

Comment 21

[Andrew Halberstadt [:ahal]]

Did a bunch of dxr'ing and getFileListing indeed looks like dead code. Removing it had no impact on the try run (bc3 is perma fail on pushes before mine too):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbaf597cec2b

Also options.test_paths will not get added to the test config because it fails the isinstance(basetring, Number) check. This is ok because without getFileListing, testPath isn't needed by JS anymore, besides changing the button label. I simply removed this because it is only mildly useful in the event someone uses --no-autorun with a single test path, which is rare.

Comment 22

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8621202 [details]
MozReview Request: Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester

https://reviewboard.mozilla.org/r/10921/#review9891

Ship It!

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92faa039f31b

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92faa039f31b