Closed Bug 1040658 Opened 6 years ago Closed 6 years ago

Add Gaia build unit tests to TBPL

Categories

(Firefox OS Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: yurenju, Assigned: jgriffin)

References

Details

(Keywords: ateam-b2g-task)

Attachments

(3 files, 1 obsolete file)

we should also add gaia build unit test to TBPL just like gaia build integration test.
I didn't even know there were such tests!

These are kicked off using 'make build-test-unit'. It looks like we can use the exact same mozharness script as for the build integration tests, but substitute the correct make target.
Keywords: ateam-b2g-task
jgriffin, I want to help on this issue, does any document/link which is you recommand to read?
Assignee: nobody → yurenju.mozilla
Flags: needinfo?(jgriffin)
I'll take a crack at this, Yuren.
Flags: needinfo?(jgriffin)
Attachment #8470294 - Flags: review?(jlund)
Assignee: yurenju.mozilla → jgriffin
Comment on attachment 8470294 [details] [diff] [review]
Add mozharness script for gaia_build_unit tests,

Review of attachment 8470294 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. see in-line comment for a higher level question that may extend the scope of this bug but I think it's worth asking since we are continuing to add more gaia scripts :)

::: scripts/gaia_build_unit.py
@@ +14,5 @@
> +from mozharness.mozilla.testing.gaia_test import GaiaTest
> +from mozharness.mozilla.testing.unittest import TestSummaryOutputParserHelper
> +
> +
> +class GaiaIntegrationTest(GaiaTest):

First time looking at these gaia scripts. IIUC, this would introduces the 4th class into mozharness named GaiaIntegrationTest that inherits from GaiaTest. I've not seen too many projects that have identical class names within it. Is this an beneficial pattern? Genuine question in case it's just me being ignorant.

Some of these files duplicate everything bar two strings: https://pastebin.mozilla.org/5918677

What's your opinion on unifying them and adding CLI options for the varying target names/options? I suppose if the classes are planning to differ quite a bit, we can keep them as separate scripts but having class names that are unique (e.g. using the filename as the class) still seems like the thing we should do.
Attachment #8470294 - Flags: review?(jlund) → review+
Comment on attachment 8470295 [details] [diff] [review]
Schedule gaia_build_unit tests on cedar,

lgtm :)
Attachment #8470295 - Flags: review?(jlund) → review+
Using the same class names is harmless, since these scripts will never be imported in the same scope.  But, it's probably a better practice to make the names more distinct, for readability if nothing else.

As far as combining similar scripts, it's a tough call.  We've done that elsewhere, but it hasn't always been that useful.  For example, the marionette script handles both marionette-webapi and gaia-ui-tests.  But the script is a bit messy as a result, which makes maintenance a bit trickier, and the code less readable; I think it would have been better to have a unique script for each.

I think the model I like the best puts common code inside mixins and provides a distinct script for each job.  The piece that's missing in this patch is some common setup for node-based jobs, so I'll refactor that into a mixin and make a new patch.
> I think the model I like the best puts common code inside mixins and
> provides a distinct script for each job.  The piece that's missing in this
> patch is some common setup for node-based jobs, so I'll refactor that into a
> mixin and make a new patch.

makes sense. tx for getting me up to speed. hit me with the new patch whenever it's ready :)
This moves a bit of common code into gaia.py, and fixes the duplicate class names.
Attachment #8473355 - Flags: review?(jlund)
Attachment #8470294 - Attachment is obsolete: true
Comment on attachment 8473355 [details] [diff] [review]
Add mozharness script for gaia_build_unit tests,

Review of attachment 8473355 [details] [diff] [review]:
-----------------------------------------------------------------

for some reason I couldn't import this cleanly but from the patch it lvgtm :)

thanks for hearing out my thoughts and cleaning up code that is outside the scope of this bug.
Attachment #8473355 - Flags: review?(jlund) → review+
Duplicate of this bug: 1054775
Comment on attachment 8473355 [details] [diff] [review]
Add mozharness script for gaia_build_unit tests,

checked in: https://hg.mozilla.org/build/mozharness/rev/0a84fdd65af9
Comment on attachment 8470295 [details] [diff] [review]
Schedule gaia_build_unit tests on cedar,

The mozharness changes look good on cypress, so landing the buildbot-configs patch:  https://hg.mozilla.org/build/buildbot-configs/rev/844939b69a05
In production with reconfig on 2014-08-19 10:47 PT
These are running on cedar, but failing.  Passing back to you Yuren.  When they're green, let me know and I'll schedule them everywhere.

link to current log:  http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/cedar-linux64_gecko/1408434314/cedar_ubuntu64_vm-b2gdt_test-gaia-build-unit-bm53-tests1-linux64-build0.txt.gz
Assignee: jgriffin → yurenju.mozilla
we got 4 errors on this run, first one we need printing more logs for investigation, others have been fixed on bug 1054822.
land pull request for printing more logs:

https://github.com/mozilla-b2g/gaia/commit/e387e95148d921fc9c33693638c1653ec6192016

:jgriffin, could we re-run again with last gaia master?
Flags: needinfo?(jgriffin)
Yes, and this runs green:  https://tbpl.mozilla.org/php/getParsedLog.php?id=46457551&tree=Cedar&full=1

I'm retriggering a couple of times to verify.
Flags: needinfo?(jgriffin)
Blocks: 1057049
Blocks: 1057050
Assignee: yurenju.mozilla → jgriffin
Tests are green on cedar; I'll now push these out to all the trunk branches.
These are running green on cedar; we can now schedule them everywhere.
Attachment #8478712 - Flags: review?(nthomas)
Attachment #8478712 - Flags: review?(nthomas) → review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.