Closed Bug 1219058 Opened 4 years ago Closed 4 years ago

Normalize Robocop source layout

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Right now, the Robocop harness and source files don't have standard Java paths.  It's no trouble for moz.build and Gradle, but there's no way to correct the package prefixes in IntelliJ (backed by Gradle).

This ticket tracks normalizing the source layouts.
Bug 1219058 - Part 1: Normalize Robocop test source layout. r?gbrown,mfinkle

This moves the Robocop test code into
src/androidTest/java/org/mozilla/gecko/tests.  The
src/androidTest/java is Gradle standard; the org/mozilla/gecko/tests
matches the package name we have now.
Attachment #8679750 - Flags: review?(mark.finkle)
Attachment #8679750 - Flags: review?(gbrown)
Bug 1219058 - Part 2: Normalize Robocop test harness source layout. r?gbrown

Pretty straight-forward.  The win here is that the directory is now
sensible, so we don't need the robocop_harness symlink for the Gradle
build configuration.
Attachment #8679751 - Flags: review?(gbrown)
Those two commits are enough to get the app/ Gradle project into the source directory, moving the Gradle and IntelliJ integration forward!  I'll push that shortly.
Comment on attachment 8679750 [details]
MozReview Request: Bug 1219058 - Part 1: Normalize Robocop test source layout. r?gbrown,mfinkle

https://reviewboard.mozilla.org/r/23521/#review20979

::: mobile/android/tests/browser/robocop/robocop.ini:4
(Diff revision 1)
> -[testGeckoProfile.java]
> +[src/androidTest/java/org/mozilla/gecko/tests/testGeckoProfile.java]

Can't we be smarter about this change? Naive-Finkle thinks we could somehow add this long path prefix to the tests in the harness.

Is there a reason we can't?
Attachment #8679750 - Flags: review?(mark.finkle)
(In reply to Mark Finkle (:mfinkle) from comment #5)
> Comment on attachment 8679750 [details]
> MozReview Request: Bug 1219058 - Part 1: Normalize Robocop test source
> layout. r?gbrown,mfinkle
> 
> https://reviewboard.mozilla.org/r/23521/#review20979
> 
> ::: mobile/android/tests/browser/robocop/robocop.ini:4
> (Diff revision 1)
> > -[testGeckoProfile.java]
> > +[src/androidTest/java/org/mozilla/gecko/tests/testGeckoProfile.java]
> 
> Can't we be smarter about this change? Naive-Finkle thinks we could somehow
> add this long path prefix to the tests in the harness.
> 
> Is there a reason we can't?

This is long but simple.  If we really care to keep this short, the best option is just to move the robocop.ini file to be next to the sources.  This is the most like the rest of the tree, and isn't surprising at all.  But it requires folks to look deep into the java/ hierarchy for a thing that has nothing to do with Java source code, effectively transferring the prefix to the tab-completion of your shell instead of the copy-paste-modify of your editor.

Given that we're all going to copy-paste-update this line, do we really care enough to keep this short?

As for alternative approaches: the build system expects tests to actually exist on disk (reasonably!)  We could hack some prefix into the manifest parser, but it's not worth the effort.
Flags: needinfo?(mark.finkle)
Comment on attachment 8679750 [details]
MozReview Request: Bug 1219058 - Part 1: Normalize Robocop test source layout. r?gbrown,mfinkle

https://reviewboard.mozilla.org/r/23521/#review21027

I hate this. I hate moving files, for the effect on hg history. I hate long paths, 'cause I'm a command-line guy.

But I don't have a better suggestion!
Attachment #8679750 - Flags: review?(gbrown) → review+
Comment on attachment 8679751 [details]
MozReview Request: Bug 1219058 - Part 2: Normalize Robocop test harness source layout. r?gbrown

https://reviewboard.mozilla.org/r/23523/#review21031

This leaves the robotium jar, README, and Makefile.in in build/mobile/robocop? That seems odd/unexpected. I think I'd prefer to see those remaining files also moved under mobile/android/...
Attachment #8679751 - Flags: review?(gbrown) → review+
(In reply to Geoff Brown [:gbrown] from comment #8)
> Comment on attachment 8679751 [details]
> MozReview Request: Bug 1219058 - Part 2: Normalize Robocop test harness
> source layout. r?gbrown
> 
> https://reviewboard.mozilla.org/r/23523/#review21031
> 
> This leaves the robotium jar, README, and Makefile.in in
> build/mobile/robocop? That seems odd/unexpected. I think I'd prefer to see
> those remaining files also moved under mobile/android/...

Aye.  Eventually I'll move them and remove build/mobile/robocop.
(In reply to Geoff Brown [:gbrown] from comment #7)
> Comment on attachment 8679750 [details]
> MozReview Request: Bug 1219058 - Part 1: Normalize Robocop test source
> layout. r?gbrown,mfinkle
> 
> https://reviewboard.mozilla.org/r/23521/#review21027
> 
> I hate this. I hate moving files, for the effect on hg history. I hate long
> paths, 'cause I'm a command-line guy.

FTR, |mach robocop testLoad| still does what you expect.

> But I don't have a better suggestion!

Pretty much :(  I don't care where the Java sources go, as long as they're in a separate directory and have the correct Java package hierarchy.

If we prefer mobile/android/robocop/src/tests, and FQNs like tests.testFoo, that's fine with me.  It doesn't keep all the tests together in mobile/android/tests (which I think is valuable) and it's not Java standard, but it's short.
Comment on attachment 8679750 [details]
MozReview Request: Bug 1219058 - Part 1: Normalize Robocop test source layout. r?gbrown,mfinkle

https://reviewboard.mozilla.org/r/23521/#review21081

Thanks for addressing my comment about the long paths. Since we don't have a simple, quick fix, we can worry about it later. Onward!
Attachment #8679750 - Flags: review+
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Flags: needinfo?(mark.finkle)
https://hg.mozilla.org/mozilla-central/rev/e65ea9918140
https://hg.mozilla.org/mozilla-central/rev/bf8e162a3580
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 44 → mozilla44
You need to log in before you can comment on or make changes to this bug.