Closed
Bug 1256984
Opened 8 years ago
Closed 8 years ago
Add a desktop notification notifying whether you're running tests in e10s or non-e10s mode
Categories
(Testing :: Mochitest, defect, P3)
Testing
Mochitest
Tracking
(e10s+, firefox48 fixed)
RESOLVED
FIXED
mozilla48
People
(Reporter: ehsan.akhgari, Unassigned)
Details
Attachments
(1 file)
4.44 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
I keep running into this mistake where I run for example mochitests without --e10s trying to test something in e10s mode and nothing on the screen makes it clear that the browser is in non-e10s mode, so I think the test passes when in reality it may not. I was thinking of adding a desktop notification that pops up when you run a test saying whether you're in e10s mode, similar to what ./mach build does when a long running build finishes. If we do it in mach, we can reuse that infrastructure <https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#485> Mike, what do you think about this? Nice, or too crazy?
Comment 2•8 years ago
|
||
I think making it clear to the tester what mode was tested is a good idea. I'm not 100% certain the long-build notification is the right choice. Perhaps we could pump out the state of what the browser was in when it was tested right at the end near the pass / fail / todo readout? That could also be useful when test logs are pasted back and forth.
Flags: needinfo?(mconley)
Updated•8 years ago
|
tracking-e10s:
--- → +
Priority: -- → P3
Reporter | ||
Comment 3•8 years ago
|
||
Makes sense. In the chrome process, we can check to see whether we're in e10s mode using gMultiProcessBrowser. Is there something similar in the content process that we can check? Anything accessible from JS would do! Thanks.
Flags: needinfo?(mconley)
Comment 4•8 years ago
|
||
Do you need SpecialPowers.isMainProcess(), perhaps?
Reporter | ||
Comment 5•8 years ago
|
||
Yes, thanks!
Reporter | ||
Comment 6•8 years ago
|
||
Attachment #8731872 -
Flags: review?(mconley)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(mconley)
Comment 7•8 years ago
|
||
Comment on attachment 8731872 [details] [diff] [review] Indicate whether tests ran in e10s mode in the mochitest-* summary Review of attachment 8731872 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/runtests.py @@ +2174,5 @@ > > if result == -1: > break > > + e10s_mode = "e10s" if options.e10s else "non-e10s" This is going to stop being true when bug 1243083 lands. We should probably alert the folks in that bug that this will need to be changed once the default shifts. ::: testing/mochitest/tests/SimpleTest/TestRunner.js @@ +448,5 @@ > } > > SpecialPowers.unregisterProcessCrashObservers(); > > + var e10sMode = SpecialPowers.isMainProcess() ? 'non-e10s' : 'e10s'; let, not var. Also, let's use double-quotes for consistency.
Attachment #8731872 -
Flags: review?(mconley) → review+
Reporter | ||
Comment 8•8 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7) > Comment on attachment 8731872 [details] [diff] [review] > Indicate whether tests ran in e10s mode in the mochitest-* summary > > Review of attachment 8731872 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: testing/mochitest/runtests.py > @@ +2174,5 @@ > > > > if result == -1: > > break > > > > + e10s_mode = "e10s" if options.e10s else "non-e10s" > > This is going to stop being true when bug 1243083 lands. We should probably > alert the folks in that bug that this will need to be changed once the > default shifts. Will do, thanks!
Comment 9•8 years ago
|
||
Also, it might be worth pushing this to try. I doubt that any of our automation attempts to parse those last few strings, but you never know - we might have just pulled the rug out from under some Python script that expects there to not be that "Mode" line. Up to you whether you want to take the gamble. :)
Reporter | ||
Comment 10•8 years ago
|
||
Already did: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cbd6acab11b Try seems to be happy!
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eafefe59f701
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•