Closed
Bug 938659
Opened 11 years ago
Closed 9 years ago
Move robocop test code into mobile/android/tests
Categories
(Firefox for Android Graveyard :: Testing, defect)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: nalexander, Assigned: nalexander)
References
Details
Attachments
(2 files, 1 obsolete file)
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•11 years ago
|
||
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•11 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
Assignee | ||
Comment 3•10 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•10 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 5•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=012daa126e02
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8550821 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 7•9 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•9 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)
Comment 9•9 years ago
|
||
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•9 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•9 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•9 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)
Comment 14•9 years ago
|
||
(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)
Updated•9 years ago
|
Flags: needinfo?(nalexander)
Comment 15•9 years ago
|
||
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•9 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•9 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)
Updated•9 years ago
|
Attachment #8550821 -
Flags: review?(mark.finkle) → review+
Comment 18•9 years ago
|
||
Comment on attachment 8550821 [details] MozReview Request: bz://938659/nalexander https://reviewboard.mozilla.org/r/2647/#review6851 Simple is good. Progress is good.
Assignee | ||
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a7c83c9a44d6 https://hg.mozilla.org/integration/fx-team/rev/9d27ff2b5d6f
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment 20•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a7c83c9a44d6 https://hg.mozilla.org/mozilla-central/rev/9d27ff2b5d6f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Assignee | ||
Comment 21•9 years ago
|
||
Attachment #8550821 -
Attachment is obsolete: true
Attachment #8618053 -
Flags: review+
Attachment #8618054 -
Flags: review+
Assignee | ||
Comment 22•9 years ago
|
||
Assignee | ||
Comment 23•9 years ago
|
||
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•