909839 - Merge JavaScript 'atoms' used by gaia and gaiatest

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Once bug 909718 is fixed we should merge the atoms used by gaia and gaiatest as these have diverged.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Are the atoms in https://github.com/mozilla-b2g/gaia/tree/master/tests/atoms still used? I believe they were for the old integration and performance tests. If they're no longer used, I suspect we can simply delete them.

Comment 2

[Julien Wajsberg [:julienw]]

I don't know, Hubert would know better by now.

Comment 3

[Hubert Figuiere [:hub]]

I do use tests/atoms/gaia_lock_screen.js for the |make test-perf|

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

There's just a minor difference between gaia_lock_screen.js in the two atoms folders, so if that's the only one in use we can probably just replace the other files in /tests/atoms with /tests/python/gaia-ui-tests/gaiatest/atoms and make gaiatest copy in from the /tests/atoms path. If there's no objections I'll work on this soon.

Comment 5

[Hubert Figuiere [:hub]]

Now that I landed bug 915156 on master, you can see what I'm using and what I'm not.

I'm also open to any refactoring to share as much as possible. Don't hesitate to ask me if you need more information.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Hubert Figuiere [:hub] from comment #5)
> Now that I landed bug 915156 on master, you can see what I'm using and what
> I'm not.
> 
> I'm also open to any refactoring to share as much as possible. Don't
> hesitate to ask me if you need more information.

Thanks Hubert, I have a patch ready that I'm testing but so far it looks good. I'll flag you for review as well as a gaia-ui-tests peer.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/14477

Pointer to Github pull-request

Comment 8

[Hubert Figuiere [:hub]]

Comment on attachment 8344049 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/14477

r+ in the fact that it doesn't touch anything from |make test-perf|

I haven't seen anything else obvious to comment on.

Comment 9

[Zac C (:zac)]

Comment on attachment 8344049 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/14477

r+

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed in:
https://github.com/mozilla-b2g/gaia/commit/2c09f900a6bd9c07891f58027ffab54ad3c176e9 (master)

This doesn't apply cleanly to v1.2, but I don't think it's necessary to uplift it.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This was backed out for causing a failure on TBPL:

https://tbpl.mozilla.org/php/getParsedLog.php?id=31743607&tree=B2g-Inbound

05:50:39     INFO -  Unpacking /builds/slave/test/gaia/tests/python/gaia-ui-tests
05:50:39     INFO -    Running setup.py egg_info for package from file:///builds/slave/test/gaia/tests/python/gaia-ui-tests
05:50:39    ERROR -      Traceback (most recent call last):
05:50:39     INFO -        File "<string>", line 14, in <module>
05:50:39     INFO -        File "/tmp/pip-L0VAOG-build/setup.py", line 34, in <module>
05:50:39     INFO -          raise Exception('JS atoms not found in %s or %s!' % (jsdir, pythondir))
05:50:39     INFO -      Exception: JS atoms not found in /tmp/pip-L0VAOG-build/../../atoms or /tmp/pip-L0VAOG-build/gaiatest/atoms!

It seems it's looking for the atoms in a tmp path instead of the gaia repo. Jonathan, any ideas?

Comment 12

[Jonathan Griffin (:jgriffin)]

The mozharness script installs using 'pip install...' which causes the Python package to be copied to a temp dir before running setup.py.   This breaks the assumptions in setup.py about the JS atoms being in ../../atoms.

The solution is to make the mozharness script call 'setup.py develop' on gaiatest, instead of installing via 'pip install'.  I'll try to find time to do this soon.

Comment 13

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #12)
> The mozharness script installs using 'pip install...' which causes the
> Python package to be copied to a temp dir before running setup.py.   This
> breaks the assumptions in setup.py about the JS atoms being in ../../atoms.
> 
> The solution is to make the mozharness script call 'setup.py develop' on
> gaiatest, instead of installing via 'pip install'.  I'll try to find time to
> do this soon.

This is actually a bit more complicated than I thought.  The version pinning mechanism implemented in bug 943008 is incompatible with the need for setup.py to copy the JS atoms from an external directory.  I think we'll need to come up with an alternate solution for that, before we can merge these atoms. :(

Comment 14

[Jonathan Griffin (:jgriffin)]

needinfo'ing myself so I can remember to look into this

Comment 15

[Jonathan Griffin (:jgriffin)]

Hmm, it appears this may work if we change 'pip install -r foo.txt .' to 'pip install -r foo.txt -e .'.

Comment 16

[Jonathan Griffin (:jgriffin)]

After bug 948788 makes it into production, it should be safe to land this again.

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15731

Updated and resubmitting now that the blocking bug is resolved.

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Local testing against my device has been successful, but I've also triggered an ad-hoc run to see how this goes in Jenkins: http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/125/

Comment 19

[Zac C (:zac)]

The launch does not appear to be a happy bunny :(

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8365983 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15731

Nope, it looks pretty bad. I'll cancel review requests for now.

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I found and fixed the issue. Running an adhoc build again to see the results before re-requesting review:
http://qa-selenium.mv.mozilla.com:8080/view/B2G Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/127/

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8365983 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15731

The adhoc build looks good. Offering up for review.

Comment 23

[Zac C (:zac)]

Comment on attachment 8365983 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15731

I am no js guru but I've reviewed the failures and they don't appear to be related. I know this is the worst kind of review but I trust jgriffin has approved the js part!

Comment 24

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Thanks! Landed in:
https://github.com/mozilla-b2g/gaia/commit/46dc0aa9c4cf3d2beef0f18f7fcbb96c8dc5ba09

Let's hope it sticks this time.

Comment 25

[Greg Weng [:snowmantw][:gweng][:λ]]

I've found that the 'atoms' would get no update even after rebase because the ignore file. And it is still used by gaia-ui-test, therefore they would cause the tests failed when these atoms contain errors. It's really strange to fix bugs inside this ignored directory, which seems deprecated or near deprecated by this bug.

I don't know if this is my own problem or not. I'll keep trying to solve the problem.

Comment 26

[Greg Weng [:snowmantw][:gweng][:λ]]

OK. After remove gaia/tests completely and rebase again, the problem seems disappeared.