966441 - Run mozbase unit tests from test package

Status: RESOLVED DUPLICATE of bug 1003417

(Release Engineering :: General, defect)

Description

[Dan Minor [:dminor]]

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

[Gregory Szorc [:gps]]

The build system relies on mozbase too. Do you mean "in addition to" instead of "instead?"

Comment 2

[Dan Minor [:dminor]]

(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

[William Lachance (:wlach)]

(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.

Comment 4

[Dan Minor [:dminor]]

(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

[Phil Ringnalda (:philor)]

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

[William Lachance (:wlach)]

(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

[Gregory Szorc [:gps]]

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

[William Lachance (:wlach)]

(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

[Gregory Szorc [:gps]]

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.

Comment 10

[Dan Minor [:dminor]]

Attachment: Add mozbase unit tests.

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

[William Lachance (:wlach)]

(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

[Jordan Lund (:jlund)]

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.

Comment 13

[Dan Minor [:dminor]]

(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

[Jordan Lund (:jlund)]

> 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+

Comment 15

[Dan Minor [:dminor]]

Thanks! Pushed to: https://hg.mozilla.org/build/mozharness/rev/3d11018ac96a

Comment 16

[Armen [:armenzg]]

Mozharness change live in production.

Comment 17

[Dan Minor [:dminor]]

Attachment: Add all_mozbase_suites to mac and windows configs

Missed defining all_mozbase_suites in the mac and windows config files first time round.

Comment 18

[Jordan Lund (:jlund)]

Comment on attachment 8376441 [details] [diff] [review]
Add all_mozbase_suites to mac and windows configs

whoops, we both missed it. lgtm :)

Comment 19

[Andrew Halberstadt [:ahal]]

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

[Andrew Halberstadt [:ahal]]

Note the mozrunner tests will land in bug 949600.

Comment 21

[Dan Minor [:dminor]]

(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.

Comment 22

[Dan Minor [:dminor]]

(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

Comment 23

[Dan Minor [:dminor]]

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

[[e-mail] (:bhearsum)]

something is in production

Comment 25

[Dan Minor [:dminor]]

Attachment: Set -b option for mozbase unit tests

As ahal mentioned in comment 19, some new tests have landed which require the -b option to be set.

Comment 26

[Andrew Halberstadt [:ahal]]

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

[Jordan Lund (:jlund)]

Comment on attachment 8381453 [details] [diff] [review]
Set -b option for mozbase unit tests

lgtm :)

Comment 28

[Dan Minor [:dminor]]

Thanks, pushed to: https://hg.mozilla.org/build/mozharness/rev/6d51f4cc75a8

Comment 29

[John Hopkins (:jhopkins)]

In production.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.'

Comment 31

[Dan Minor [:dminor]]

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.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Well, it's the same for latest builds on Cedar:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35538753&tree=Cedar&full=1

Comment 33

[Dan Minor [:dminor]]

Unassigning myself so someone else can pick it up if they're interested.