Move robocop test code into mobile/android/tests

RESOLVED FIXED in Firefox 40

Status

()

Firefox for Android
Testing
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

Trunk
Firefox 40
All
Android
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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?).
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?
(Assignee)

Comment 2

5 years ago
(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.
Component: General → Testing
Blocks: 709230
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 6

4 years ago
Created attachment 8550821 [details]
MozReview Request: bz://938659/nalexander
Attachment #8550821 - Flags: review?(mark.finkle)
(Assignee)

Comment 7

4 years ago
/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
(Assignee)

Comment 8

4 years ago
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.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(gbrown)
Thanks. Looks good to me.

If you do another try push, use "-u all,robocop-1" to trigger x86 robocop tests.
Flags: needinfo?(gbrown)
(Assignee)

Comment 10

4 years ago
(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
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?
Flags: needinfo?(michael.l.comella)
(Assignee)

Comment 12

4 years ago
(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.
(Assignee)

Comment 13

4 years ago
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}.
Flags: needinfo?(mark.finkle)
(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

?
Flags: needinfo?(mark.finkle)
Flags: needinfo?(nalexander)
Comment on attachment 8550821 [details]
MozReview Request: bz://938659/nalexander

Waiting for confirmation of NI and a new patch
Attachment #8550821 - Flags: review?(mark.finkle)
(Assignee)

Comment 16

3 years ago
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/
Attachment #8550821 - Flags: review?(mark.finkle)
(Assignee)

Comment 17

3 years ago
(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.
Flags: needinfo?(nalexander)
Attachment #8550821 - Flags: review?(mark.finkle) → review+
Comment on attachment 8550821 [details]
MozReview Request: bz://938659/nalexander

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

Simple is good. Progress is good.
(Assignee)

Updated

3 years ago
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/a7c83c9a44d6
https://hg.mozilla.org/mozilla-central/rev/9d27ff2b5d6f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
(Assignee)

Comment 21

3 years ago
Comment on attachment 8550821 [details]
MozReview Request: bz://938659/nalexander
Attachment #8550821 - Attachment is obsolete: true
Attachment #8618053 - Flags: review+
Attachment #8618054 - Flags: review+
(Assignee)

Comment 22

3 years ago
Created attachment 8618053 [details]
MozReview Request: Bug 938659 - Part 1: hg mv mobile/android/base/tests mobile/android/tests/browser/robocop. r=mfinkle
(Assignee)

Comment 23

3 years ago
Created attachment 8618054 [details]
MozReview Request: Bug 938659 - Part 2: build system changes. r=mfinkle
You need to log in before you can comment on or make changes to this bug.