Closed
Bug 929654
Opened 11 years ago
Closed 3 years ago
IDE support for building and running Robocop tests
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
Firefox 35
People
(Reporter: bnicholson, Unassigned)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [keep open])
Attachments
(11 files, 2 obsolete files)
16.02 KB,
patch
|
jmaher
:
review+
gbrown
:
review+
|
Details | Diff | Splinter Review |
2.70 KB,
patch
|
jmaher
:
review-
gbrown
:
feedback+
|
Details | Diff | Splinter Review |
1.75 KB,
patch
|
jmaher
:
review-
|
Details | Diff | Splinter Review |
1.28 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
2.60 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
11.20 KB,
patch
|
jmaher
:
review-
|
Details | Diff | Splinter Review |
3.03 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
4.46 KB,
patch
|
jmaher
:
review-
|
Details | Diff | Splinter Review |
1.91 KB,
patch
|
jmaher
:
review-
|
Details | Diff | Splinter Review |
5.30 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
We should create a separate IDE project for Robocop tests, which will allow us to build and run tests directly from the IDE.
Comment 1•10 years ago
|
||
This patch does two things. First, it unifies the test activity creation process, in preparation for future uses. Second, it adds new arguments -- "quit" and "finish" -- to the test activity destruction process. "quit" uses Robocop:Quit to kill Fennec and "finish" finishes all activities started by test code. Alternate test launchers (like from IDEs) don't have a good mechanism to pass arguments, so the default must be suitable for such mechanisms, and the best default for local testers is to not quit Fennec and to not finish activities. Therefore, these arguments default to "0" (off) and are set to "1" (on) in runtestsremote.py; this preserves the existing behaviour in automation.
Comment 2•10 years ago
|
||
This patch stops pushing the obsolete fennec_ids.txt file to the testing device and the stale robotium.config file after the profile it references is deleted.
Comment 3•10 years ago
|
||
Comment 4•10 years ago
|
||
Comment 5•10 years ago
|
||
On my local testing devices, |adb pull| complains about not being able to read /data/anr/traces.txt but the command succeeds. The subsequent local file read produces an IOError. This ignores that exception.
Comment 6•10 years ago
|
||
Before a Robocop test starts, |recordLogcat| clears the |adb logcat| on device. If a Robocop test fails, we print the logcat immediately after the test. The moved call avoids printing the logcat after all tests have completed, which avoids printing a spurious second copy. We still print the logcat after running a Mochitest.
Comment 7•10 years ago
|
||
Comment 8•10 years ago
|
||
This adds an activity to the Robocop package that starts Fennec. This is needed because we pass configuration via robotium.config, so the configuration needs to be interpreted by something running on device; and it shouldn't be anything in Fennec itself. This new activity starts Fennec with the appropriate configuration. The test harness then waits forever.
Comment 9•10 years ago
|
||
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
Comment on attachment 8481693 [details] [diff] [review] Part 1: Prepare test activity setUp and tearDown for IDE launching. r=jmaher Review of attachment 8481693 [details] [diff] [review]: ----------------------------------------------------------------- Hi Joel, hi Geoff, Here are a bunch (!) of small commits that make local Robocop testing smoother for me. They fix small things, re-organize code, prune logging, and add flags that allow to write a small |mach robocop| command. (Patch upcoming on Bug 874729). These also let me run Robocop tests from Eclipse; I'll document this with a blog post when I get some time. I haven't requested all 11 reviews. Can you two decide who's right for each part? Historically, I think Joel has reviewed other things in this area.
Attachment #8481693 -
Flags: review?(jmaher)
Attachment #8481693 -
Flags: review?(gbrown)
Comment 13•10 years ago
|
||
mcomella: this is relevant to your interests, I think.
Flags: needinfo?(michael.l.comella)
Comment 14•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8d879af8c241 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8d879af8c241
Comment 15•10 years ago
|
||
Comment on attachment 8481693 [details] [diff] [review] Part 1: Prepare test activity setUp and tearDown for IDE launching. r=jmaher Review of attachment 8481693 [details] [diff] [review]: ----------------------------------------------------------------- these changes look fine. We would need to modify Talos as well, for example: http://hg.mozilla.org/build/talos/file/49b74c08dad4/talos/test.py#l153
Attachment #8481693 -
Flags: review?(jmaher) → review+
Comment 16•10 years ago
|
||
I see some bad things in the on-device logcats, but I'd like to get feedback on the other 10 (!) parts.
Updated•10 years ago
|
Attachment #8481694 -
Flags: review?(jmaher)
Updated•10 years ago
|
Attachment #8481695 -
Flags: review?(jmaher)
Updated•10 years ago
|
Attachment #8481696 -
Flags: review?(jmaher)
Updated•10 years ago
|
Attachment #8481697 -
Flags: review?(jmaher)
Updated•10 years ago
|
Attachment #8481698 -
Flags: review?(jmaher)
Updated•10 years ago
|
Attachment #8481699 -
Flags: review?(jmaher)
Updated•10 years ago
|
Attachment #8481700 -
Flags: review?(jmaher)
Comment 17•10 years ago
|
||
Comment on attachment 8481694 [details] [diff] [review] Part 2: Stop pushing fennec_ids.txt; delete stale robotium.config files. r=jmaher Review of attachment 8481694 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/runtestsremote.py @@ -677,5 @@ > - dm.removeFile(os.path.join(deviceRoot, "fennec_ids.txt")) > - fennec_ids = os.path.abspath(os.path.join(SCRIPT_DIR, "fennec_ids.txt")) > - if not os.path.exists(fennec_ids) and options.robocopIds: > - fennec_ids = options.robocopIds > - dm.pushFile(fennec_ids, os.path.join(deviceRoot, "fennec_ids.txt")) so why don't we need fennec_ids anymore? if we don't need it for some reason, we should remove the copying of it in talos as well: http://hg.mozilla.org/build/talos/file/tip/talos/ttest.py#l217
Attachment #8481694 -
Flags: review?(jmaher) → review-
Updated•10 years ago
|
Attachment #8481701 -
Flags: review?(jmaher)
Updated•10 years ago
|
Attachment #8481702 -
Flags: review?(jmaher)
Comment 18•10 years ago
|
||
Comment on attachment 8481695 [details] [diff] [review] Part 3: Turn off keyguard and turn on screen when waiting for Gecko:Ready. r=jmaher Review of attachment 8481695 [details] [diff] [review]: ----------------------------------------------------------------- this is nice, but what about our other android automation? I feel we are solving this in the wrong place.
Attachment #8481695 -
Flags: review?(jmaher) → review-
Updated•10 years ago
|
Attachment #8481702 -
Flags: review?(jmaher)
Updated•10 years ago
|
Attachment #8481703 -
Flags: review?(jmaher)
Updated•10 years ago
|
Attachment #8481696 -
Flags: review?(jmaher) → review+
Updated•10 years ago
|
Attachment #8481697 -
Flags: review?(jmaher) → review+
Comment 19•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #17) > Comment on attachment 8481694 [details] [diff] [review] > Part 2: Stop pushing fennec_ids.txt; delete stale robotium.config files. > r=jmaher > > Review of attachment 8481694 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: testing/mochitest/runtestsremote.py > @@ -677,5 @@ > > - dm.removeFile(os.path.join(deviceRoot, "fennec_ids.txt")) > > - fennec_ids = os.path.abspath(os.path.join(SCRIPT_DIR, "fennec_ids.txt")) > > - if not os.path.exists(fennec_ids) and options.robocopIds: > > - fennec_ids = options.robocopIds > > - dm.pushFile(fennec_ids, os.path.join(deviceRoot, "fennec_ids.txt")) > > so why don't we need fennec_ids anymore? if we don't need it for some > reason, we should remove the copying of it in talos as well: > http://hg.mozilla.org/build/talos/file/tip/talos/ttest.py#l217 This has been filed for some time: https://bugzilla.mozilla.org/show_bug.cgi?id=969921 But I don't have write access to build/talos, making it not worth my time to fix. We should get rid of this in pieces; there's no reason to require this all at once.
Updated•10 years ago
|
Attachment #8481698 -
Flags: review?(jmaher) → review+
Updated•10 years ago
|
Attachment #8481699 -
Flags: review?(jmaher) → review+
Comment 20•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #18) > Comment on attachment 8481695 [details] [diff] [review] > Part 3: Turn off keyguard and turn on screen when waiting for Gecko:Ready. > r=jmaher > > Review of attachment 8481695 [details] [diff] [review]: > ----------------------------------------------------------------- > > this is nice, but what about our other android automation? I feel we are > solving this in the wrong place. In automation, I don't think this will actually do anything, because aren't these things all headless? And not key-guarded? This is solely best effort for local testers. I'm happy to drop it (since it only helps on certain devices for me) or build it into devicemanager or similar, and call it from an appropriate place. Suggestions?
Updated•10 years ago
|
Attachment #8481702 -
Flags: review?(jmaher)
Comment 21•10 years ago
|
||
Comment on attachment 8481700 [details] [diff] [review] Part 8: Implement --no-autorun for Robocop. r=jmaher Review of attachment 8481700 [details] [diff] [review]: ----------------------------------------------------------------- I don't like --no-autorun and a 7 day default value. ::: testing/mochitest/mochitest_options.py @@ +65,5 @@ > + { "action": "store_false", > + "dest": "autorun", > + "help": "do not start running tests when the application starts", > + "default": False, > + }], We have --autorun and --no-autorun? why not use --autorun properly? ::: testing/mochitest/runtestsremote.py @@ +737,5 @@ > + # never returns. > + options.browserArgs = ["start", > + "-n", "org.mozilla.roboexample.test/org.mozilla.gecko.LaunchFennecWithConfigurationActivity", > + "&&", "cat"] > + dm.default_timeout = 7 * 24 * 60 * 60 # One week, in seconds. one week? why not 10 minutes?
Attachment #8481700 -
Flags: review?(jmaher) → review-
Updated•10 years ago
|
Attachment #8481701 -
Flags: review?(jmaher) → review+
Comment 22•10 years ago
|
||
Comment on attachment 8481702 [details] [diff] [review] Part 10: Implement --no-logcat-on-fail for Robocop. r=jmaher Review of attachment 8481702 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/runtestsremote.py @@ +115,5 @@ > + dest = "logcatOnFail", > + help = "Log |adb logcat| output after each failing test.") > + self.add_option("--no-logcat-on-fail", action = "store_false", > + dest = "logcatOnFail", > + help = "Do not log |adb logcat| output after a failing test.") we really should just use --logcat-on-fail and be smart about it, these double cli options seem hacky.
Attachment #8481702 -
Flags: review?(jmaher) → review-
Comment 23•10 years ago
|
||
Comment on attachment 8481703 [details] [diff] [review] Part 11: Implement --test-name for testing multiple tests in Robocop. r=jmaher Review of attachment 8481703 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/runtestsremote.py @@ +120,5 @@ > defaults["logcatOnFail"] = True > + > + self.add_option("--test-name", action = "append", > + type = "string", dest = "testNames", > + help = "") please add a help message here to indicate this is for robocop only! Can we not use --test-path instead?
Attachment #8481703 -
Flags: review?(jmaher) → review-
Comment 24•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #21) > Comment on attachment 8481700 [details] [diff] [review] > Part 8: Implement --no-autorun for Robocop. r=jmaher > > Review of attachment 8481700 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't like --no-autorun and a 7 day default value. > > ::: testing/mochitest/mochitest_options.py > @@ +65,5 @@ > > + { "action": "store_false", > > + "dest": "autorun", > > + "help": "do not start running tests when the application starts", > > + "default": False, > > + }], > > We have --autorun and --no-autorun? why not use --autorun properly? Explain to me how to use autorun properly? We autorun both mochitest-remote and mochitest-robocop, even though it's not passed to the harness. So we're ignoring it now, and not passing it (AFAICT). Do you mean I should make sure it's passed properly (I don't know where the right places are) and then act on it? I am happy to do this with guidance. > ::: testing/mochitest/runtestsremote.py > @@ +737,5 @@ > > + # never returns. > > + options.browserArgs = ["start", > > + "-n", "org.mozilla.roboexample.test/org.mozilla.gecko.LaunchFennecWithConfigurationActivity", > > + "&&", "cat"] > > + dm.default_timeout = 7 * 24 * 60 * 60 # One week, in seconds. > > one week? why not 10 minutes? I'm sorry, I didn't give this good context. The purpose of this is to let you "run Fennec in testing mode" and keep the mochi.test web server up, while you rebuild and redeploy the Robocop APK from an IDE. That is, in a terminal, I run |mach robocop --serve| and then I can develop my tests in Eclipse. Killing the mochi.test server after 10 minutes doesn't serve this goal. I'm planning a blog post and screen recording to show how this will work, which I will prioritize a bit higher.
Comment 25•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #22) > Comment on attachment 8481702 [details] [diff] [review] > Part 10: Implement --no-logcat-on-fail for Robocop. r=jmaher > > Review of attachment 8481702 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: testing/mochitest/runtestsremote.py > @@ +115,5 @@ > > + dest = "logcatOnFail", > > + help = "Log |adb logcat| output after each failing test.") > > + self.add_option("--no-logcat-on-fail", action = "store_false", > > + dest = "logcatOnFail", > > + help = "Do not log |adb logcat| output after a failing test.") > > we really should just use --logcat-on-fail and be smart about it, these > double cli options seem hacky. I agree, but our existing code ignores existing flags as it sees fit, and I don't know the entire set of consumers. Same as above. Again, happy to address with guidance.
Comment 26•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #23) > Comment on attachment 8481703 [details] [diff] [review] > Part 11: Implement --test-name for testing multiple tests in Robocop. > r=jmaher > > Review of attachment 8481703 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: testing/mochitest/runtestsremote.py > @@ +120,5 @@ > > defaults["logcatOnFail"] = True > > + > > + self.add_option("--test-name", action = "append", > > + type = "string", dest = "testNames", > > + help = "") > > please add a help message here to indicate this is for robocop only! Can we > not use --test-path instead? --test-path, unfortunately, only allows for one path. It's used by both Mochitest and Robocop, so I didn't think I could change it to be an append variable. If you think that's okay, I'll happily do it...
Comment 27•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #20) > (In reply to Joel Maher (:jmaher) from comment #18) > > Comment on attachment 8481695 [details] [diff] [review] > > Part 3: Turn off keyguard and turn on screen when waiting for Gecko:Ready. > > r=jmaher > > > > Review of attachment 8481695 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > this is nice, but what about our other android automation? I feel we are > > solving this in the wrong place. > > In automation, I don't think this will actually do anything, because aren't > these things all headless? And not key-guarded? This is solely best effort > for local testers. I'm happy to drop it (since it only helps on certain > devices for me) or build it into devicemanager or similar, and call it from > an appropriate place. > > Suggestions? good point. I would prefer all automation we do on devices that adjusts device settings be done as a pre-requisite to running tests and be documented somewhere. With that said, maybe we just put this in and file a bug to solve this more generically in devicemanager.
Updated•10 years ago
|
Flags: needinfo?(jmaher)
Comment 28•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #27) > good point. I would prefer all automation we do on devices that adjusts > device settings be done as a pre-requisite to running tests and be > documented somewhere. With that said, maybe we just put this in and file a > bug to solve this more generically in devicemanager. +1 -- let's spin this topic off into a separate bug. The other place we deal with this is the Watcher service, http://hg.mozilla.org/mozilla-central/annotate/7753c52d5976/build/mobile/sutagent/android/watcher/WatcherService.java#l299. Watcher works great in automation but is generally not used by developers. I like the idea of a generic devicemanager solution, but I do not immediately see how that can be implemented.
Comment 29•10 years ago
|
||
Comment on attachment 8481693 [details] [diff] [review] Part 1: Prepare test activity setUp and tearDown for IDE launching. r=jmaher Review of attachment 8481693 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tests/BaseTest.java @@ +43,5 @@ > import android.support.v4.app.FragmentActivity; > import android.support.v4.app.FragmentManager; > import android.text.TextUtils; > import android.util.DisplayMetrics; > +import android.util.Log; I would like to see a comment here, "// Use mAsserter logging instead of android.util.Log unless absolutely required". @@ +170,5 @@ > + if ("1".equals(FennecInstrumentationTestRunner.getFennecArguments().getString("quit", killFennecDefault))) { > + // Request the browser force quit and wait for it to take effect. > + Log.i(LOGTAG, "Requesting force quit."); > + GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Robocop:Quit", null)); > + mSolo.sleep(ROBOCOP_QUIT_WAIT_MS); I see some crashes on shutdown in your try run. I *think* that is an existing problem, but you should check that your changes are not contributing. ::: mobile/android/base/tests/UITest.java @@ +8,5 @@ > > import org.mozilla.gecko.Actions; > import org.mozilla.gecko.Assert; > import org.mozilla.gecko.Driver; > +import org.mozilla.gecko.FennecInstrumentationTestRunner; It looks like this import is not necessary. ::: testing/mochitest/runtestsremote.py @@ +718,5 @@ > options.app = "am" > + options.browserArgs = ["instrument", "-w", > + "-e", "deviceroot", deviceRoot, > + "-e", "quit", "1", > + "-e", "finish", "1", A comment here about the meanings of "quit" and "finish" might be useful.
Attachment #8481693 -
Flags: review?(gbrown) → review+
Comment 30•10 years ago
|
||
Comment on attachment 8481694 [details] [diff] [review] Part 2: Stop pushing fennec_ids.txt; delete stale robotium.config files. r=jmaher Review of attachment 8481694 [details] [diff] [review]: ----------------------------------------------------------------- I think this is fine. I would like to see all the other fennec_ids references cleaned up as soon as possible...before we forget about it again!
Attachment #8481694 -
Flags: feedback+
Comment 31•10 years ago
|
||
Comment on attachment 8481696 [details] [diff] [review] Part 4: Fail earlier for local testers who can't reach mochi.test. r=jmaher Review of attachment 8481696 [details] [diff] [review]: ----------------------------------------------------------------- Is there much benefit here? If mochi.test is not reachable, won't most tests fail within seconds? ::: mobile/android/base/tests/BaseRobocopTest.java @@ +133,5 @@ > + if (200 != statusCode) { > + throw new IllegalStateException("Status code: " + statusCode); > + } > + } catch (Exception e) { > + mAsserter.ok(false, "Could not HTTP GET from " + rawUrl + "; aborting.", e.toString()); There might be an opportunity here to point developers to "the real problem"...adding something like "Is there a network path from this host (...) to the test device? Did the test web server (xpcshell) start?"
Attachment #8481696 -
Flags: feedback+
Comment 32•10 years ago
|
||
Comment on attachment 8481700 [details] [diff] [review] Part 8: Implement --no-autorun for Robocop. r=jmaher Review of attachment 8481700 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/runtestsremote.py @@ +737,5 @@ > + # never returns. > + options.browserArgs = ["start", > + "-n", "org.mozilla.roboexample.test/org.mozilla.gecko.LaunchFennecWithConfigurationActivity", > + "&&", "cat"] > + dm.default_timeout = 7 * 24 * 60 * 60 # One week, in seconds. Maybe something like maxint would be best. Does everything get cleaned up eventually? Does the device end up accumulating "cat" processes?
Comment 33•10 years ago
|
||
Comment on attachment 8481701 [details] [diff] [review] Part 9: Implement --no-screenshot-on-fail for Robocop. r=jmaher Review of attachment 8481701 [details] [diff] [review]: ----------------------------------------------------------------- Dumping screenshots (and logcats) when running tests locally clutters the output, and bugs me. But I sometimes think there might still be value there: Rather that suppressing these features, maybe they should be shunted to separate files? ::: testing/mochitest/mochitest_options.py @@ +396,5 @@ > "default": False, > "dest": "screenshotOnFail", > "help": "Take screenshots on all test failures. Set $MOZ_UPLOAD_DIR to a directory for storing the screenshots." > }], > + [["--no-screenshot-on-fail"], I think it should be possible to get by with the existing --screenshot-on-fail option: Pass that option in automation; don't pass it if screenshots are not needed.
Don't think I currently have anything to add here.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Flags: needinfo?(michael.l.comella)
Comment 35•10 years ago
|
||
of course I cannot remember the pending reason for the needinfo, but I recall something about where to do the device settings, gbrown weighs in on that subject as well in comment 28. We shouldn't block on that, we can adjust settings here, file a bug with a list of settings in watcher, sutagent, setup instructions, ide, devicemanager- then figure out what is needed for automation vs developers and find the right place.
Flags: needinfo?(jmaher)
Comment 36•10 years ago
|
||
I'm taking our discussion of fennec_ids.txt and gbrown's f+ as positive review for Part 2. I landed Parts 2, 5, 6, and 7 as parts of Bug 874729. I'll push on the flags and |mach robocop-test --serve| on this ticket when I get time.
Comment 37•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cabc4055bf53
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Comment 38•10 years ago
|
||
Argh, sorry folks, this hasn't all landed. The previous patch was intended to be part of Bug 874729.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 39•10 years ago
|
||
I think this is the update you're looking for in: https://bugzilla.mozilla.org/show_bug.cgi?id=1068489#c5
Attachment #8481696 -
Attachment is obsolete: true
Attachment #8497874 -
Flags: review?(nalexander)
Updated•10 years ago
|
Whiteboard: [keep open]
Comment 40•10 years ago
|
||
Comment on attachment 8497874 [details] [diff] [review] Part 4: Fail earlier for local testers who can't reach mochi.test. r=nalexander This patch looks good, but I expect you'll hit the same failures in Talos tests that the original did. (See https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8d879af8c241.) Can you push a try build (full unit tests) and/or investigate what's happening in Talos above?
Attachment #8497874 -
Flags: review?(nalexander) → feedback+
Comment 41•10 years ago
|
||
Let's find out :-) https://tbpl.mozilla.org/?tree=Try&rev=dd1f6d548fb2
Comment 42•10 years ago
|
||
This seems like a reasonable solution to allow us to enhance local test useability. https://tbpl.mozilla.org/?tree=Try&rev=20a62b663ad6 (After the try run, I tweaked some comments in this attached patch).
Attachment #8497874 -
Attachment is obsolete: true
Attachment #8498394 -
Flags: review?(nalexander)
Comment 43•10 years ago
|
||
Comment on attachment 8498394 [details] [diff] [review] Part 4: Fail earlier for local testers who can't reach mochi.test. r=nalexander Review of attachment 8498394 [details] [diff] [review]: ----------------------------------------------------------------- Add the caveat about non-Talos tests in the commit message (header line or body, either is fine). Thanks for picking this up! ::: mobile/android/base/tests/BaseRobocopTest.java @@ +119,5 @@ > + * with diagnostic information. Not currently available for TALOS tests, which are rarely run > + * locally in any case. > + */ > + public void throwIfHttpGetFails() { > + if (getTestType() == Type.TALOS) { Hmm. Whatever. @@ +136,5 @@ > + if (200 != statusCode) { > + throw new IllegalStateException("Status code: " + statusCode); > + } > + } catch (Exception e) { > + mAsserter.ok(false, "Robocop tests need wifi / network access (HTTP GET from " + rawUrl + ").", e.toString()); nit: from "..." failed. And it's not true that the Robocop tests need general WiFi or network access. They just need to be able to reach the mochi.test server. How about: "Robocop tests need to be able to reach " + rawUrl.
Attachment #8498394 -
Flags: review?(nalexander) → review+
Comment 44•10 years ago
|
||
re: |from "..." failed.| ... I didn't want to add that to the message, as it could be a TEST-PASS or TEST-UNEXPECTED-FAIL ... having it on a TEST-PASS message seemed odd (?) re: |"Robocop tests need to be able to reach " + rawUrl.| ... That seems overly technical ... a new dev will run into that issue if his Android wifi is Off, or of course there's some other connectivity error. I was hoping the FAIL messages might provide more helpful diagnostics / solutions (That's why in the first version I was checking the results, then issuing multi-line / verbose messages). Is |"Robocop tests need wifi to be able to reach " + rawUrl.| any better?
Comment 45•10 years ago
|
||
Tweaked the fail message: TEST-UNEXPECTED-FAIL | testSelectionHandler | Robocop tests on your device need network/wifi access to reach: [http://192.168.2.5:8888/tests]. - org.apache.http.conn.HttpHostConnectException: Connection to http://192.168.2.5:8888 refused https://hg.mozilla.org/integration/fx-team/rev/63061f730d01
Blocks: 1256454
Assignee: nalexander → nobody
Assignee: nobody → nalexander
QA Contact: sdaswani
Updated•6 years ago
|
Assignee: nalexander → nobody
Updated•6 years ago
|
QA Contact: sdaswani
Comment 47•3 years ago
|
||
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 3 years ago
Resolution: --- → INCOMPLETE
Assignee | ||
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
•