938659 - Move robocop test code into mobile/android/tests

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Nick Alexander :nalexander [he/him]]

Right now we have harness code in build/mobile/robocop and test code in mobile/android/base/tests.  The test code is read when we build build/mobile/robocop.

I think we should split this:

* build/mobile/robocop will build a harness JAR (that does *not* reference Fennec at all);
* mobile/android/tests/browser/robocop will build the robocop-debug APK (that is built after Fennec and does reference the Fennec jars).

While we're here, we should rename all the lowercase test*.java files to sane Test*.java files.  And I think the build/mobile/robocop harness code is using the org.mozilla.gecko package; it should use something else (org.mozilla.robocop? org.mozilla.testharness?).

Comment 1

[Geoff Brown [:gbrown]]

To be clear: 

Would the harness JAR be added to the robocop APK? (I wouldn't want to introduce another APK.)

If we rename testBookmark.java to TestBookmark.java, I assume we would rename the testBookmark class to TestBookmark. Would we also rename the testBookmark() function to TestBookmark(), and if so, would robotium pick that up as a test function?

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Geoff Brown [:gbrown] from comment #1)
> To be clear: 
> 
> Would the harness JAR be added to the robocop APK? (I wouldn't want to
> introduce another APK.)

Yes.  We're going to be adding APKs for background JUnit 3 tests and probably browser JUnit 3 tests anyway; they might use the same harness JAR and get treated just like Robocop.

> If we rename testBookmark.java to TestBookmark.java, I assume we would
> rename the testBookmark class to TestBookmark.

Yes.

 Would we also rename the
> testBookmark() function to TestBookmark(), and if so, would robotium pick
> that up as a test function?

I can't find a reference right now, but Robotium recognizes test* as test functions.  (Like JUnit.)  So no, we wouldn't rename test functions.  Our test function names are just conventions, anyway, since Robotium happily runs multiple test* named functions.

Comment 3

[Nick Alexander :nalexander [he/him]]

Here's a try build that moves everything Robocop into mobile/android/tests/browser/robocop.  It's not looking that healthy, but it's unclear if the failures that match intermittents are real or actually intermittent.  Retriggers all around.  This doesn't do anything to separate the harness from the Robocop test files.

https://tbpl.mozilla.org/?tree=Try&rev=a492efc2906f

Comment 4

[Nick Alexander :nalexander [he/him]]

The orange looks to be that I didn't bump the roboextender build, which is awkwardly placed in $TOPSRCDIR/testing/mochitest.

https://tbpl.mozilla.org/?tree=Try&rev=00fbf356d7cf

Comment 5

[Nick Alexander :nalexander [he/him]]

https://tbpl.mozilla.org/?tree=Try&rev=012daa126e02

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: bz://938659/nalexander

Comment 7

[Nick Alexander :nalexander [he/him]]

/r/2649 - Bug 938659 - Part 1: Move mobile/android/base/tests to mobile/android/tests/browser/robocop. r=mfinkle
/r/2651 - Bug 938659 - Part 2: Move Robocop source files to Java-standard paths. r=mfinkle

Pull down these commits:

hg pull review -r 012daa126e02b58b312e80e614da3726c4766f73

Comment 8

[Nick Alexander :nalexander [he/him]]

mcomella, gbrown: I don't expect this to cause issues, but I do you want you both to be aware of this change and have a chance to highlight any issues I may be missing.

Comment 9

[Geoff Brown [:gbrown]]

Thanks. Looks good to me.

If you do another try push, use "-u all,robocop-1" to trigger x86 robocop tests.

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Geoff Brown [:gbrown] from comment #9)
> Thanks. Looks good to me.
> 
> If you do another try push, use "-u all,robocop-1" to trigger x86 robocop
> tests.

Thanks, I did this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50bac9d5a602

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Why androidTest in tests/browser/robocop/src/androidTest/...? Doesn't robocop imply androidTest?

Also, the tests were not yet capitalized, as per comment 0 - is this still expected to be done? Follow-up?

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #11)
> Why androidTest in tests/browser/robocop/src/androidTest/...? Doesn't
> robocop imply androidTest?

It does semantically, but this is how the Android-Gradle plugin arranges things:
src/{main,debug,release,flavour}/{java,res} for code and assets;
src/{test,androidTest}/{java,res} for unit and integration tests;
etc.
 
> Also, the tests were not yet capitalized, as per comment 0 - is this still
> expected to be done? Follow-up?

Yeah, and it doesn't trigger a message in IJ like in Eclipse, so it's less irritating.  That requires code changes so it's harder to review.  Follow-up.

Comment 13

[Nick Alexander :nalexander [he/him]]

mfinkle: can you make a call here?  The only requirements are that we move m/a/b/tests out of a directory containing Java sources (so out of the tree rooted at m/a/b), and that we move the files into a tree with suffix org/mozilla/gecko/tests.  "Gradle standard" is closest to

{prefix}/src/androidTest/{package}

but the minimum useful is

{prefix}/{src or java}/{package}.

A long time ago we decided to root these tests at prefix=m/a/tests/browser/robocop, but we can shorten if we want.  If we really want to be short but not in any way standard, we could go

mobile/android/robocop/{src or java}.

Comment 14

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

(In reply to Nick Alexander :nalexander from comment #13)
> mfinkle: can you make a call here?  The only requirements are that we move
> m/a/b/tests out of a directory containing Java sources (so out of the tree
> rooted at m/a/b), and that we move the files into a tree with suffix
> org/mozilla/gecko/tests.  "Gradle standard" is closest to
> 
> {prefix}/src/androidTest/{package}
> 
> but the minimum useful is
> 
> {prefix}/{src or java}/{package}.
> 
> A long time ago we decided to root these tests at
> prefix=m/a/tests/browser/robocop, but we can shorten if we want.  If we
> really want to be short but not in any way standard, we could go
> 
> mobile/android/robocop/{src or java}.

I remember the decision to root at mobile/android/tests/browser/robocop and I'm still cool with that. Given what you're telling me, we'd end up with:

mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests

?

Comment 15

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

Comment on attachment 8550821 [details]
MozReview Request: bz://938659/nalexander

Waiting for confirmation of NI and a new patch

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8550821 [details]
MozReview Request: bz://938659/nalexander

/r/2649 - Bug 938659 - Part 1: hg mv mobile/android/base/tests mobile/android/tests/browser/robocop. r=mfinkle
/r/2651 - Bug 938659 - Part 2: build system changes. r=mfinkle

Pull down these commits:

hg pull -r 75b5b060d9c2733efbd56580a5828b3feac7c417 https://reviewboard-hg.mozilla.org/gecko/

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #14)
> (In reply to Nick Alexander :nalexander from comment #13)
> > mfinkle: can you make a call here?  The only requirements are that we move
> > m/a/b/tests out of a directory containing Java sources (so out of the tree
> > rooted at m/a/b), and that we move the files into a tree with suffix
> > org/mozilla/gecko/tests.  "Gradle standard" is closest to
> > 
> > {prefix}/src/androidTest/{package}
> > 
> > but the minimum useful is
> > 
> > {prefix}/{src or java}/{package}.
> > 
> > A long time ago we decided to root these tests at
> > prefix=m/a/tests/browser/robocop, but we can shorten if we want.  If we
> > really want to be short but not in any way standard, we could go
> > 
> > mobile/android/robocop/{src or java}.
> 
> I remember the decision to root at mobile/android/tests/browser/robocop and
> I'm still cool with that. Given what you're telling me, we'd end up with:
> 
> mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests
> 
> ?

OK, I've pushed a slightly different approach to review and try (see review link for try link).  This just moves m/a/b/tests to m/a/tests/browser/robocop.  For the foreseeable future we're not going to avoid objdir symlinks in the Gradle configuration, so there's no pressing need to make files have more Java-like paths.  (That is, the pushed Gradle configuration does the right thing in IntelliJ.)  We could do the renaming (to what you suggest above) if we think it's better to one-shot the changes.

Comment 18

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

Comment on attachment 8550821 [details]
MozReview Request: bz://938659/nalexander

https://reviewboard.mozilla.org/r/2647/#review6851

Simple is good. Progress is good.

Comment 19

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/a7c83c9a44d6
https://hg.mozilla.org/integration/fx-team/rev/9d27ff2b5d6f

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7c83c9a44d6
https://hg.mozilla.org/mozilla-central/rev/9d27ff2b5d6f

Comment 22

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 938659 - Part 1: hg mv mobile/android/base/tests mobile/android/tests/browser/robocop. r=mfinkle

Comment 23

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 938659 - Part 2: build system changes. r=mfinkle