Closed Bug 1171971 Opened 4 years ago Closed 4 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(1 file)

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.
Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester
Attachment #8621202 - Flags: review?(cmanchester)
Attachment #8621202 - Flags: review?(cmanchester)
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.
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.
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.
(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.
(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.
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.
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.
(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.
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 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
Attachment #8621202 - Flags: review?(cmanchester)
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.
Attachment #8621202 - Flags: review?(cmanchester)
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 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
Attachment #8621202 - Flags: review?(cmanchester)
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.
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.
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.
Ah, I didn't notice that, thanks.
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 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
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 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!
Attachment #8621202 - Flags: review?(cmanchester) → review+
https://hg.mozilla.org/mozilla-central/rev/92faa039f31b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.