Closed
Bug 966441
Opened 11 years ago
Closed 8 years ago
Run mozbase unit tests from test package
Categories
(Release Engineering :: General, defect)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 1003417
People
(Reporter: dminor, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
(deleted),
patch
|
jlund
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jlund
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jlund
:
review+
|
Details | Diff | Splinter Review |
We should run the mozbase unit tests from the test package instead of during "make check" to help avoid problems like Bug 963885.
Comment 1•11 years ago
|
||
The build system relies on mozbase too. Do you mean "in addition to" instead of "instead?"
Reporter | ||
Comment 2•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #1)
> The build system relies on mozbase too. Do you mean "in addition to" instead
> of "instead?"
The intention was "instead" but if we need to run these tests to validate that we can build, then maybe this is not worth pursuing.
Comment 3•11 years ago
|
||
(In reply to Dan Minor [:dminor] from comment #2)
> (In reply to Gregory Szorc [:gps] from comment #1)
> > The build system relies on mozbase too. Do you mean "in addition to" instead
> > of "instead?"
>
> The intention was "instead" but if we need to run these tests to validate
> that we can build, then maybe this is not worth pursuing.
No, we don't need to run these tests to validate that we can build. Some of mozbase's functionality is used by the build system, but by no means all. We really want to seperate out the concerns here so that we can validate changes to mozbase are ok without bothering the sheriffs with hard-to-diagnose build failures. If a mozbase change is checked in that breaks the build, that should be obvious enough on its own.
Reporter | ||
Comment 4•11 years ago
|
||
(In reply to William Lachance (:wlach) from comment #3)
> (In reply to Dan Minor [:dminor] from comment #2)
> > (In reply to Gregory Szorc [:gps] from comment #1)
> > > The build system relies on mozbase too. Do you mean "in addition to" instead
> > > of "instead?"
> >
> > The intention was "instead" but if we need to run these tests to validate
> > that we can build, then maybe this is not worth pursuing.
>
> No, we don't need to run these tests to validate that we can build. Some of
> mozbase's functionality is used by the build system, but by no means all. We
> really want to seperate out the concerns here so that we can validate
> changes to mozbase are ok without bothering the sheriffs with
> hard-to-diagnose build failures. If a mozbase change is checked in that
> breaks the build, that should be obvious enough on its own.
Good, the mozharness work is done and I'll be putting it up for review shortly pending a run on Ash.
Comment 5•11 years ago
|
||
Well, you could make a case for needing to run some tests in make check since the build slaves are completely separate from and different than the test slaves - if the DeviceManager tests that I shut you off over only really matter on a device, then running them on the Linux build slave is pointless, but if a mozbase change makes us do something incorrectly on a Linux build slave building Fennec, testing that on Tegras and Pandas is pointless. You could also hypothetically introduce wrongness on Win2K8 but not WinXP or Win7 or Win8, or on 10.7 but not 10.6 or 10.8. Dunno how likely wrongness that doesn't break the build entirely is, though.
Comment 6•11 years ago
|
||
(In reply to Phil Ringnalda (:philor) from comment #5)
> Well, you could make a case for needing to run some tests in make check
> since the build slaves are completely separate from and different than the
> test slaves - if the DeviceManager tests that I shut you off over only
> really matter on a device, then running them on the Linux build slave is
> pointless, but if a mozbase change makes us do something incorrectly on a
> Linux build slave building Fennec, testing that on Tegras and Pandas is
> pointless. You could also hypothetically introduce wrongness on Win2K8 but
> not WinXP or Win7 or Win8, or on 10.7 but not 10.6 or 10.8. Dunno how likely
> wrongness that doesn't break the build entirely is, though.
In practice a change to mozbase is much more likely to break a test harness in some hard-to-diagnose way than the build. Or at least that's been my experience so far. Always open revisiting my assumptions with new data...
BTW, there are currently no mozbase/mozdevice tests that run against/on a tegra or panda (we simulate such a device on the machine running the test). Though now that I think about it, that might not be a bad thing to add...
Comment 7•11 years ago
|
||
I've been considering the possibility of running the build/Python tests in automation *before* we build. Otherwise, failures from building will mask test failures and it won't be clear we're dealing with detectable build system bustage because said tests won't run unless the build is successful. I would likely lump mozbase tests into this pre-build test step because mozbase regressions can regress the build in non-obvious manners.
Comment 8•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #7)
> I've been considering the possibility of running the build/Python tests in
> automation *before* we build. Otherwise, failures from building will mask
> test failures and it won't be clear we're dealing with detectable build
> system bustage because said tests won't run unless the build is successful.
> I would likely lump mozbase tests into this pre-build test step because
> mozbase regressions can regress the build in non-obvious manners.
If you want to run some subset of the mozbase tests before you build, that's fine with me. I still maintain we need a separate job for *all* the mozbase tests because of things like bug 963885.
It's important that we be able to continue to run things like the mozdevice tests, even though they are not currently 100% deterministic and strictly have nothing to do with the build. I don't see any other way of doing this while keeping the builds sheriffable.
Comment 9•11 years ago
|
||
Re-read comment 1: I'm not opposed to running mozbase tests on non-build slaves. I do have reservations about not running some of them on build slaves.
Reporter | ||
Comment 10•11 years ago
|
||
I've also switched to using the MozbaseMixin which we've been using on the pandas for several months now.
I did an Ash run here: https://tbpl.mozilla.org/?tree=Ash&rev=22a4b0c6dea4. It is noisy, but it looks like the desktop unittests are green where they ran.
Comment 11•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #9)
> Re-read comment 1: I'm not opposed to running mozbase tests on non-build
> slaves. I do have reservations about not running some of them on build
> slaves.
Ok, so I guess it makes sense to file some kind of followup/dependant bug to do what you suggest (run a restricted subset of mozbase unit tests before the build). Do we need to block moving the mozbase tests to a seperate job on that? Or should we just run the mozbase tests in both places for now?
Comment 12•11 years ago
|
||
Comment on attachment 8374012 [details] [diff] [review]
Add mozbase unit tests.
Review of attachment 8374012 [details] [diff] [review]:
-----------------------------------------------------------------
r+ once my Q's are explained are not cause for concern. :)
::: mozharness/mozilla/testing/unittest.py
@@ +90,5 @@
> + self.pass_count = int(summary_match_list[-1])
> + else:
> + # some suites do not report number of successes
> + self.pass_count = 1
> + self.fail_count = 0
So I am not sure what's happening here. Do we add self.fail_count because we are not expecting a 'FAILED' line summary for mozbase tests? I'm just worried by setting fail_count to 0 here, it might overwrite/contradict the fail_group condition below.
::: scripts/desktop_unittest.py
@@ -195,5 @@
> - 'mozcrash', 'mozinstall', 'mozdevice', 'mozprofile', 'mozprocess',
> - 'mozrunner'):
> - self.register_virtualenv_module(m, url=os.path.join(mozbase_dir,
> - m))
> -
please help ignorant me out. The mock module replaces 'mozfile' 'mozlog' 'mozinfo' etc...? Just trying to figure out why we can now rm all the modules from being added to virtualenv now.
@@ +250,5 @@
> "\nIf you meant to have options for this suite, "
> "please make sure they are specified in your "
> "config under %s_options" %
> (suite_category, suite_category))
> + return base_cmd
bah, sorry you had to add that. That look's like my fault. I think I missed it till now because we always had options for suites.
Attachment #8374012 -
Flags: review?(jlund) → review+
Reporter | ||
Comment 13•11 years ago
|
||
(In reply to Jordan Lund (:jlund) from comment #12)
> Comment on attachment 8374012 [details] [diff] [review]
> Add mozbase unit tests.
>
> Review of attachment 8374012 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r+ once my Q's are explained are not cause for concern. :)
>
> ::: mozharness/mozilla/testing/unittest.py
> @@ +90,5 @@
> > + self.pass_count = int(summary_match_list[-1])
> > + else:
> > + # some suites do not report number of successes
> > + self.pass_count = 1
> > + self.fail_count = 0
>
> So I am not sure what's happening here. Do we add self.fail_count because we
> are not expecting a 'FAILED' line summary for mozbase tests? I'm just
> worried by setting fail_count to 0 here, it might overwrite/contradict the
> fail_group condition below.
There will only ever be a 'OK' or 'FAILURE' line for the mozbase tests, so I need this for this suite. This won't cause problems with any of our existing suites, but could trip someone up in the future. Would you like me to comment on this or use a different approach?
>
> ::: scripts/desktop_unittest.py
> @@ -195,5 @@
> > - 'mozcrash', 'mozinstall', 'mozdevice', 'mozprofile', 'mozprocess',
> > - 'mozrunner'):
> > - self.register_virtualenv_module(m, url=os.path.join(mozbase_dir,
> > - m))
> > -
>
> please help ignorant me out. The mock module replaces 'mozfile' 'mozlog'
> 'mozinfo' etc...? Just trying to figure out why we can now rm all the
> modules from being added to virtualenv now.
>
The mozfile, mozlog, mozinfo stuff is replaced by using MozbaseMixin in the class. The MozbaseMixin contains this same code and is used by the android script already.
Comment 14•11 years ago
|
||
> There will only ever be a 'OK' or 'FAILURE' line for the mozbase tests, so I
> need this for this suite. This won't cause problems with any of our existing
> suites, but could trip someone up in the future. Would you like me to
> comment on this or use a different approach?
ahh, I see. I don't see a major issue since this is in a separate condition. A comment though does sound like a good idea. Just in case someone, as you said, adds a similar test that is this format.
> The mozfile, mozlog, mozinfo stuff is replaced by using MozbaseMixin in the
> class. The MozbaseMixin contains this same code and is used by the android
> script already.
:) I guess I should have looked at MOzbaseMixin more... lgtm r+
Reporter | ||
Comment 15•11 years ago
|
||
Thanks! Pushed to: https://hg.mozilla.org/build/mozharness/rev/3d11018ac96a
Comment 16•11 years ago
|
||
Mozharness change live in production.
Reporter | ||
Comment 17•11 years ago
|
||
Missed defining all_mozbase_suites in the mac and windows config files first time round.
Attachment #8376441 -
Flags: review?(jlund)
Comment 18•11 years ago
|
||
Comment on attachment 8376441 [details] [diff] [review]
Add all_mozbase_suites to mac and windows configs
whoops, we both missed it. lgtm :)
Attachment #8376441 -
Flags: review?(jlund) → review+
Comment 19•11 years ago
|
||
There are some new mozrunner tests that will only run if a binary is passed into test.py, e.g python test.py -b path/to/firefox. We should configure platforms that have a simple binary (e.g not emulators/tegras) to pass this in.
Might be better to file a follow up for this though.
Comment 20•11 years ago
|
||
Note the mozrunner tests will land in bug 949600.
Reporter | ||
Comment 21•11 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #19)
> There are some new mozrunner tests that will only run if a binary is passed
> into test.py, e.g python test.py -b path/to/firefox. We should configure
> platforms that have a simple binary (e.g not emulators/tegras) to pass this
> in.
>
> Might be better to file a follow up for this though.
Thanks for the heads up. If 949600 lands before this is closed, I'll fix it here, otherwise I'll do a follow up.
Reporter | ||
Comment 22•11 years ago
|
||
(In reply to Jordan Lund (:jlund) from comment #18)
> Comment on attachment 8376441 [details] [diff] [review]
> Add all_mozbase_suites to mac and windows configs
>
> whoops, we both missed it. lgtm :)
Thanks, pushed to https://hg.mozilla.org/build/mozharness/rev/4fc68a913f08
Reporter | ||
Comment 23•11 years ago
|
||
We have test failures on linux due to 'ifconfig' not being in the path on the linux test machines:
https://tbpl.mozilla.org/php/getParsedLog.php?id=34850784&tree=Cedar&full=1#error0
Looks green on windows and os x.
Comment 24•11 years ago
|
||
something is in production
Reporter | ||
Comment 25•11 years ago
|
||
As ahal mentioned in comment 19, some new tests have landed which require the -b option to be set.
Attachment #8381453 -
Flags: review?(jlund)
Comment 26•11 years ago
|
||
Nice. The mozrunner tests should also be runnable on b2g desktop builds, but it's probably ok to just get them working on firefox desktop for now.
Comment 27•11 years ago
|
||
Comment on attachment 8381453 [details] [diff] [review]
Set -b option for mozbase unit tests
lgtm :)
Attachment #8381453 -
Flags: review?(jlund) → review+
Reporter | ||
Comment 28•11 years ago
|
||
Thanks, pushed to: https://hg.mozilla.org/build/mozharness/rev/6d51f4cc75a8
Comment 29•11 years ago
|
||
In production.
Comment 30•11 years ago
|
||
John, shouldn't the latest inbound landings run those mozrunner tests now? When I check the log files they are still skipped because the binary has not been specified. Is something missing here still?
https://tbpl.mozilla.org/php/getParsedLog.php?id=35587777&tree=Mozilla-Inbound&full=1
test_run_interactive (test_interactive.MozrunnerInteractiveTestCase)
Bug 965183: Run process in interactive mode and call wait() ... skipped 'No binary has been specified.'
Flags: needinfo?(jhopkins)
Reporter | ||
Comment 31•11 years ago
|
||
Henrik, this bug is about getting things running from the test package and is only active on the Cedar branch right now, so nothing here should affect inbound.
Flags: needinfo?(jhopkins)
Comment 32•11 years ago
|
||
Well, it's the same for latest builds on Cedar:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35538753&tree=Cedar&full=1
Reporter | ||
Comment 33•10 years ago
|
||
Unassigning myself so someone else can pick it up if they're interested.
Assignee: dminor → nobody
Status: ASSIGNED → NEW
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Updated•6 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•