1130872 - Add Robocop JavascriptTest verifying that AppConstants module can be imported and is sensible

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

This ticket tracks verifying that Bug 1130812 does not regress.  It's a great mentor bug, although not quite a good first bug.

What we want is a JavascriptTest (like [1], but there are lots of examples) that imports AppConstants.jsm and verifies that a few constants are reasonable.  That might include checking that ANDROID_PACKAGE_NAME is a non-empty string and perhaps that the build timestamp is actually set.

[1] See the pair of
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testAccounts.java
and
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testAccounts.js

Comment 1

[Robin Ricard [:rricard]]

Hi Nick !

I think I can help on this ! I worked on a similar JSTest w/ margaret previously (see bug 1015395 and bug 1128287). So I think it is indeed a good next bug for me.

Assign me to it if you think I can help.

Thanks !

Comment 2

[Robin Ricard [:rricard]]

Attachment: Not yet tested (clobber was needed ... so it takes time) but should do the trick !

I launch a try as soon as you have a positive feedback !

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8563778 [details] [diff] [review]
Not yet tested (clobber was needed ... so it takes time) but should do the trick !

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

Works for me!  Push a try build for warm fuzzy green feelings :)

Comment 4

[Robin Ricard [:rricard]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7e860e05780

Comment 5

[Robin Ricard [:rricard]]

Comment on attachment 8563778 [details] [diff] [review]
Not yet tested (clobber was needed ... so it takes time) but should do the trick !

Green build (apart from an another intermittent bug) !

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8563778 [details] [diff] [review]
Not yet tested (clobber was needed ... so it takes time) but should do the trick !


>+add_task(function* testAppConstants() {
>+  do_check_true(AppConstants.ANDROID_PACKAGE_NAME.length > 0);

Can we add this too, so we verify the substitution actually happened:

 do_check_true(AppConstants.ANDROID_PACKAGE_NAME != "@ANDROID_PACKAGE_NAME@");

Comment 7

[Robin Ricard [:rricard]]

I do that as soon as I get to my computer

Comment 8

[Robin Ricard [:rricard]]

Attachment: Add Robocop JavascriptTest verifying that AppConstants module can be imported and is sensible

Can't start a try: tree is closed

Comment 9

[Robin Ricard [:rricard]]

The try, finally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce821f10c501

Comment 10

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/33ca49e58e4b

Comment 11

[Nick Alexander :nalexander [he/him]]

rricard: I pushed a slightly different version that uses do_check_neq.  The advantage is that do_check_neq prints its arguments on failure, which will show what AppConstants.ANDROID_PACKAGE_NAME is (null, "", garbage, etc).

Thanks for the patch and a green try build!  I don't have a ton of JS mentored bugs right now, but if you're interested in a Java bug, the following is a small one: Bug 1124193.  It might be too easy, though -- in which case ask in #mobile for a better ticket.  Thanks again!

Comment 12

[Robin Ricard [:rricard]]

Thanks for the advice, I'll may take it !

Comment 13

[Robin Ricard [:rricard]]

I could help on the Java side too !

Comment 14

[Robin Ricard [:rricard]]

I also thought of working on bug 1065969 too... I think I'll do them sequentially

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33ca49e58e4b