Closed
Bug 1171971
Opened 9 years ago
Closed 9 years ago
Mochitest move 'test_paths' out of mach and into MochitestArgumentParser; remove --test-path
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
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.
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1171971 - Move test_paths argument out of mach and into mochitest; remove --test-path, r=chmanchester
Attachment #8621202 -
Flags: review?(cmanchester)
Updated•9 years ago
|
Attachment #8621202 -
Flags: review?(cmanchester)
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
(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.
Assignee | ||
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
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•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
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.
Assignee | ||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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)
Assignee | ||
Comment 13•9 years ago
|
||
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.
Assignee | ||
Comment 14•9 years ago
|
||
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)
Assignee | ||
Comment 15•9 years ago
|
||
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.
Assignee | ||
Comment 16•9 years ago
|
||
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•9 years ago
|
||
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.
Assignee | ||
Comment 18•9 years ago
|
||
Ah, I didn't notice that, thanks.
Assignee | ||
Comment 19•9 years ago
|
||
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.
Assignee | ||
Comment 20•9 years ago
|
||
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
Assignee | ||
Comment 21•9 years ago
|
||
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•9 years ago
|
||
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+
Comment 23•9 years ago
|
||
Comment 24•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•