Closed Bug 1130872 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: nalexander, Assigned: rricard, Mentored)

References

Details

(Whiteboard: [lang=js][good next bug])

Attachments

(1 file, 1 obsolete file)

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
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 !
Flags: needinfo?(nalexander)
I launch a try as soon as you have a positive feedback !
Attachment #8563778 - Flags: feedback?(nalexander)
Flags: needinfo?(nalexander)
Assignee: nobody → ricard.robin
Status: NEW → ASSIGNED
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 :)
Attachment #8563778 - Flags: feedback?(nalexander) → feedback+
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) !
Attachment #8563778 - Flags: review?(nalexander)
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@");
I do that as soon as I get to my computer
Can't start a try: tree is closed
Attachment #8563778 - Attachment is obsolete: true
Attachment #8563778 - Flags: review?(nalexander)
Attachment #8564194 - Flags: review?(nalexander)
The try, finally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce821f10c501
Flags: needinfo?(nalexander)
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!
Flags: needinfo?(nalexander)
Attachment #8564194 - Flags: review?(nalexander) → review+
Thanks for the advice, I'll may take it !
I could help on the Java side too !
I also thought of working on bug 1065969 too... I think I'll do them sequentially
https://hg.mozilla.org/mozilla-central/rev/33ca49e58e4b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.