Closed Bug 1184186 Opened 4 years ago Closed 4 years ago

Consider converting some all-js robocop tests to mochitest-chrome

Categories

(Firefox for Android :: Testing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: gbrown, Assigned: gbrown)

References

Details

Attachments

(22 files, 1 obsolete file)

1.06 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
4.25 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
8.28 KB, patch
ally
: review+
Details | Diff | Splinter Review
6.75 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
5.74 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
11.89 KB, patch
myk
: review+
Details | Diff | Splinter Review
5.21 KB, patch
jryans
: review+
Details | Diff | Splinter Review
10.95 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
12.27 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
12.74 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
11.88 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
7.46 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
4.98 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
5.07 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
12.88 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
10.55 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
6.35 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
18.59 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
18.43 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
8.87 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
17.21 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
14.68 KB, patch
nalexander
: review+
markh
: feedback+
Details | Diff | Splinter Review
I think some JavascriptTest-based Robocop tests can be easily converted to mochitest-chrome. The mochitest-chrome versions should run faster (less setup/cleanup overhead) and produce more readable logs.
Attached patch wip (obsolete) — Splinter Review
Some JavascriptTest-based Robocop tests have a non-trivial Java component and are NOT good candidates for this work; these include:

x testFilePicker
x testFindInPage
x testOrderedBroadcast
x testSelectionCarets
x testTrackingProtection
x testUITelemetry

That leaves these Javascript tests which may be good candidates:

testAboutLogins
testAccounts
testAndroidLog
testAppConstants
testBrowserDiscovery
testDebuggerServer
testDesktopUserAgent
testDeviceSearchEngine
testHistoryService
testHomeProvider
testJavaAddons
testJNI
testMigrateUI
testMozPay
testNetworkManager
testOfflinePage
testReaderView
testReadingListCache
testResourceSubstitutions
testRestrictedProfiles
testSessionFormData
testSharedPreferences
testSimpleDiscovery
testVideoControls
testVideoDiscovery
testWebChannel
I am using "gen_template.pl --type chrome" to generate a chrome test template, then importing the robocop js file into the template html.  Some mechanical changes affect most tests:

 - do_check_true -> ok
 - do_check_eq -> is
 - s/do_print/dump/
 - add_task/run_next_test are not available; remove or use Task.spawn if necessary
 - remove old .java/.js and update robocop.ini; add new file, update chrome.ini.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d0356cac6da
 - s/do_register_cleanup/SimpleTest.registerCleanupFunction/
Attachment #8634174 - Attachment is obsolete: true
(In reply to Geoff Brown [:gbrown] (pto July 22 - 27) from comment #3)
>  - s/do_print/dump/

That works, in that it dumps to the logcat, but

s/do_print/info/

has the advantage that the message is also dumped to the test log, if appropriate (on failure, or if requestCompleteLog() is called and verbose logging enabled).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8847db12359

I have successfully converted 21 robocop tests to mochitest-chrome; they run in under 2 minutes on mochitest-chrome -- a big time savings.

There are a few more tests that don't quite work; some of those have the same intermittent failures in robocop.

To do (after pto): clean up my translation of add_task; review remaining accesses to robocop resources and move as necessary; re-consider whether some of these would be better suited to browser-chrome; kick off some reviews!

I really like the local-run experience:

gbrown@mozpad:~/src$ ./mach mochitest mobile/android/tests/browser/chrome --log-mach-verbose
...
######
### Now running mochitest-chrome.
######

 0:00.87 LOG: MainThread INFO Android sdk version '18'; will use this to filter manifests
 0:01.09 LOG: MainThread INFO Checking for orphan ssltunnel processes...
 0:01.14 LOG: MainThread INFO Checking for orphan xpcshell processes...
 0:01.19 SUITE_START: MainThread 21
 0:01.27 LOG: MainThread INFO pushing /home/gbrown/objdirs/droid/_tests/testing/mochitest/chrome to /sdcard/tests/chrome on device...
pk12util: PKCS12 IMPORT SUCCESSFUL
 0:12.86 LOG: MainThread INFO MochitestServer : launching [u'/home/gbrown/objdirs/firefox/dist/bin/xpcshell', '-g', '/home/gbrown/objdirs/firefox/dist/bin', '-v', '170', '-f', '/home/gbrown/objdirs/firefox/dist/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpZW8_s9.mozrunner'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '192.168.0.81'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', '/home/gbrown/objdirs/droid/_tests/testing/mochitest/server.js']
 0:12.86 LOG: MainThread INFO runtests.py | Server pid: 15949
 0:12.88 LOG: MainThread INFO runtests.py | Websocket server pid: 15952
 0:12.89 LOG: MainThread INFO runtests.py | SSL tunnel pid: 15973
 0:16.64 LOG: MainThread INFO runtests.py | Running tests: start.

INFO | automation.py | Application pid: 3027
 0:34.47 LOG:  INFO SimpleTest START
 0:34.55 TEST_START:  mobile/android/tests/browser/chrome/test_about_logins.html
 0:39.44 TEST_END:  OK
 0:39.79 TEST_START:  mobile/android/tests/browser/chrome/test_accounts.html
 0:40.26 TEST_END:  OK
 0:40.43 TEST_START:  mobile/android/tests/browser/chrome/test_android_log.html
 0:40.77 TEST_END:  OK
 0:40.92 TEST_START:  mobile/android/tests/browser/chrome/test_app_constants.html
 0:41.22 TEST_END:  OK
 0:41.37 TEST_START:  mobile/android/tests/browser/chrome/test_debugger_server.html
 0:43.03 TEST_END:  OK
 0:43.33 TEST_START:  mobile/android/tests/browser/chrome/test_desktop_useragent.html
 0:46.81 TEST_END:  OK
 0:47.17 TEST_START:  mobile/android/tests/browser/chrome/test_device_search_engine.html
 0:49.28 TEST_END:  OK
 0:50.60 TEST_START:  mobile/android/tests/browser/chrome/test_home_provider.html
 0:52.14 TEST_END:  OK
 0:52.31 TEST_START:  mobile/android/tests/browser/chrome/test_java_addons.html
 0:53.26 TEST_END:  OK
 0:53.57 TEST_START:  mobile/android/tests/browser/chrome/test_jni.html
 0:53.96 TEST_END:  OK
 0:54.11 TEST_START:  mobile/android/tests/browser/chrome/test_migrate_ui.html
 0:54.60 TEST_END:  OK
 0:54.84 TEST_START:  mobile/android/tests/browser/chrome/test_network_manager.html
 0:55.17 TEST_END:  OK
 0:55.35 TEST_START:  mobile/android/tests/browser/chrome/test_offline_page.html
 0:59.50 TEST_END:  OK
 0:59.64 TEST_START:  mobile/android/tests/browser/chrome/test_reader_view.html
 1:02.97 TEST_END:  OK
 1:03.20 TEST_START:  mobile/android/tests/browser/chrome/test_resource_substitutions.html
 1:03.69 TEST_END:  OK
 1:03.87 TEST_START:  mobile/android/tests/browser/chrome/test_restricted_profiles.html
 1:04.43 TEST_END:  OK
 1:04.67 TEST_START:  mobile/android/tests/browser/chrome/test_session_form_data.html
 1:23.72 TEST_END:  OK
 1:23.92 TEST_START:  mobile/android/tests/browser/chrome/test_shared_preferences.html
 1:25.10 TEST_END:  OK
 1:25.25 TEST_START:  mobile/android/tests/browser/chrome/test_simple_discovery.html
 1:25.61 TEST_END:  OK
 1:25.76 TEST_START:  mobile/android/tests/browser/chrome/test_video_discovery.html
 1:28.34 TEST_END:  OK
 1:28.56 TEST_START:  mobile/android/tests/browser/chrome/test_web_channel.html
 1:33.95 TEST_END:  OK
INFO | automation.py | Application ran for: 0:01:35.691279
INFO | zombiecheck | Reading PID log: /tmp/tmpwcrNRipidlog
MOZ_UPLOAD_DIR not defined; tombstone check skipped
 1:53.50 LOG: MainThread INFO Stopping web server
 1:53.53 LOG: MainThread INFO Stopping web socket server
 1:53.55 LOG: MainThread INFO Stopping ssltunnel
 1:53.57 LOG: MainThread INFO WARNING | leakcheck | refcount logging is off, so leaks can't be detected!
 1:53.57 LOG: MainThread INFO runtests.py | Running tests: end.
 1:34.28 LOG:  INFO TEST-START | Shutdown
 1:34.29 LOG:  INFO Passed:  174
 1:34.29 LOG:  INFO Failed:  0
 1:34.30 LOG:  INFO Todo:    0
 1:34.31 LOG:  INFO Slowest: 19046ms - chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_session_form_data.html
 1:34.31 LOG:  INFO SimpleTest FINISHED
 1:55.68 SUITE_END: MainThread 
Summary
=======

Ran 21 tests
Expected results: 21
Unexpected results: 0

OK
For the record, I'm quite pleased to see tests moving out of the Robocop harness.
A really simple start to a flood of patches: Add an initially-empty chrome.ini for mobile, at mobile/android/tests/browser/chrome/chrome.ini. Note that these tests will be run on Android, skipped on all other platforms.
Attachment #8640202 - Flags: review?(nalexander)
And here's the very simplest JavascriptTest, testAppConstants, converted to mochitest-chrome.

I intend to r? the original author of each converted test. In this case, that's :rricard, but he hasn't been active in bugzilla lately.

Drive-by comments/reviews by others welcome, especially on these first few patches.

The html wrapper form is pretty standard for chrome tests and was generated by gen_template.pl, as recommended at https://developer.mozilla.org/en/docs/Mochitest#Test_templates.

I removed the license notice since it is not included in most mochitest-chrome tests, and is not generated by gen_template.pl; I'm happy to put it back in if that's preferred.
Attachment #8640208 - Flags: review?(nalexander)
Note that I've moved the check for AppConstants.NIGHTLY_BUILD into javascript. Otherwise, the robocop -> mochitest-chrome conversion is pretty straight forward.
Attachment #8640213 - Flags: review?(ally)
Comment on attachment 8640208 [details] [diff] [review]
convert testAppConstants

Review of attachment 8640208 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.  I don't know much about mochitest-chrome: is there a pattern for multiple test functions?  Is the pattern to extract multiple test files?
Attachment #8640208 - Flags: review?(nalexander) → review+
> I removed the license notice since it is not included in most
> mochitest-chrome tests, and is not generated by gen_template.pl; I'm happy
> to put it back in if that's preferred.

This sounds good: we should do whatever the majority of the existing tests do.
(In reply to Nick Alexander :nalexander from comment #11)
> lgtm.  I don't know much about mochitest-chrome: is there a pattern for
> multiple test functions?  Is the pattern to extract multiple test files?

Are you thinking of something like the xpcshell add_test() and add_task()? Those appear to not be available. Many mochitests seem to keep each test file pretty simple, with a straight-forward main-line and perhaps a few helper functions.
Comment on attachment 8640213 [details] [diff] [review]
convert testAboutLogins

Review of attachment 8640213 [details] [diff] [review]:
-----------------------------------------------------------------

I have some questions, the biggest one being about hitting the network using test links. Assuming that all checks out,r+.

::: mobile/android/tests/browser/chrome/test_about_logins.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

needs a license block

@@ +76,5 @@
> +
> +    SimpleTest.finish();
> +  }
> +
> +  if (AppConstants.NIGHTLY_BUILD) {

I'll be removing the nightly only build flags so soon that aboutlogins can ride the trains. Would I just delete this if block?

@@ +86,5 @@
> +
> +  </script>
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1136477">Mozilla Bug 1136477</a>

dont we usually use local links for testing? So that the test harness doesnt hit the network when we run the test suites...
Attachment #8640213 - Flags: review?(ally) → review+
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #14)
> ::: mobile/android/tests/browser/chrome/test_about_logins.html
> @@ +1,1 @@
> > +<!DOCTYPE HTML>
> 
> needs a license block

See Comment 12. Do you have a strong opinion?
 
> @@ +76,5 @@
> > +
> > +    SimpleTest.finish();
> > +  }
> > +
> > +  if (AppConstants.NIGHTLY_BUILD) {
> 
> I'll be removing the nightly only build flags so soon that aboutlogins can
> ride the trains. Would I just delete this if block?

Yes.

> @@ +86,5 @@
> > +
> > +  </script>
> > +</head>
> > +<body>
> > +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1136477">Mozilla Bug 1136477</a>
> 
> dont we usually use local links for testing? So that the test harness doesnt
> hit the network when we run the test suites...

Yes, we always use local links for testing to avoid network reliance.

This link is a mochitest convention: Provide a link on the test page back to the bug that introduced the test, as a convenience to manual testers / people debugging or reading this test in future. I think it is of limited usefulness, especially on mobile, but it also does no harm: The link is not used by the test and would only be followed if someone manually clicked on the link.
This is the first of my patches to use add_task. I said earlier that add_task() was not available, but that is changing with bug 1187701. There's still some discussion happening in that bug, but the initial patch posted in bug 1187701 is sufficient for my purposes.
Attachment #8641237 - Flags: review?(nalexander)
Depends on: 1187701
This is a simple conversion of testAccounts to mochitest-chrome.

One issue: The original test called Accounts.launchSetup(), I suppose just to verify that the call does not cause a crash? In the mochitest environment, I don't want to call that without cleaning up, for fear of affecting later tests. Is it OK to leave it out? Or, is there a good way to close the setup wizard?
Attachment #8641240 - Flags: review?(rnewman)
Attachment #8641241 - Flags: review?(myk)
Attachment #8641243 - Flags: review?(jryans)
I added some closeTab calls to clean up; otherwise, I think this is a simple robocop -> mochitest conversion.
Attachment #8641246 - Flags: review?(mark.finkle)
Comment on attachment 8641237 [details] [diff] [review]
convert testResourceSubstitutions

Review of attachment 8641237 [details] [diff] [review]:
-----------------------------------------------------------------

wfm if it works for you.

::: mobile/android/tests/browser/chrome/test_resource_substitutions.html
@@ +49,5 @@
> +    // This can be any file that we know exists in the root of every APK.
> +    let packageName = yield readChannel("resource://android/package-name.txt");
> +    info(packageName);
> +
> +    // It's difficult to fish ANDROID_PACKAGE_NAME from JavaScript, so we test the

It's now easy to do this (https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/AppConstants.jsm#179) if you want to strengthen the test.
Attachment #8641237 - Flags: review?(nalexander) → review+
Attachment #8641248 - Flags: review?(mark.finkle)
Attachment #8641246 - Flags: review?(mark.finkle) → review+
Attachment #8641248 - Flags: review?(mark.finkle) → review+
Attachment #8641243 - Flags: review?(jryans) → review+
Attachment #8641799 - Flags: review?(margaret.leibovic)
Attachment #8641801 - Flags: review?(nalexander)
Attached patch convert testJNISplinter Review
Attachment #8641802 - Flags: review?(nalexander)
Attachment #8641803 - Flags: review?(margaret.leibovic)
Attachment #8641804 - Flags: review?(mark.finkle)
Attachment #8641805 - Flags: review?(mark.finkle)
Attachment #8641806 - Flags: review?(margaret.leibovic)
Attachment #8641807 - Flags: review?(mark.finkle)
Attachment #8641808 - Flags: review?(mark.finkle)
Attachment #8641809 - Flags: review?(nalexander)
Attachment #8641810 - Flags: review?(mark.finkle)
Attachment #8641814 - Flags: review?(mark.finkle)
Attachment #8641815 - Flags: review?(nalexander)
A big "thank you" (and "I'm sorry!") to all reviewers. I hope it goes without saying: If for any reason you don't want your robocop test converted, just say so. Nits and rubber-stamps accepted equally.
I also considered but decided not to convert:

testBrowserDiscovery
testHistoryService
testReadingListCache
testVideoControls

These can be converted, but have intermittent failures in both the robocop and chrome versions; I want to keep the mochitest-chrome job as green as possible. If the intermittent failures can be addressed, these are all good candidates for mochitest-chrome in the future.
Attachment #8641814 - Flags: review?(mark.finkle) → review+
Attachment #8641810 - Flags: review?(mark.finkle) → review+
Attachment #8641808 - Flags: review?(mark.finkle) → review+
Comment on attachment 8641807 [details] [diff] [review]
convert testRestrictedProfiles

Restricted Profiles have been changing, but it looks like you have the most recent test code.
Attachment #8641807 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #39)
> Restricted Profiles have been changing, but it looks like you have the most
> recent test code.

Yes, I recently updated for 8fd046b77ea3.
Attachment #8641804 - Flags: review?(mark.finkle) → review+
Comment on attachment 8641805 [details] [diff] [review]
convert testOfflinePage

Kinda sad I re-used video_controls.html for this, but I guess that test might be converted at some point too :)
Attachment #8641805 - Flags: review?(mark.finkle) → review+
Comment on attachment 8641802 [details] [diff] [review]
convert testJNI

Review of attachment 8641802 [details] [diff] [review]:
-----------------------------------------------------------------

rs.
Attachment #8641802 - Flags: review?(nalexander) → review+
Comment on attachment 8641801 [details] [diff] [review]
convert testJavaAddons

Review of attachment 8641801 [details] [diff] [review]:
-----------------------------------------------------------------

If it works for you, rubber stamp from me.
Attachment #8641801 - Flags: review?(nalexander) → review+
Comment on attachment 8641809 [details] [diff] [review]
convert testSharedPreferences

Review of attachment 8641809 [details] [diff] [review]:
-----------------------------------------------------------------

rs.
Attachment #8641809 - Flags: review?(nalexander) → review+
Comment on attachment 8641241 [details] [diff] [review]
convert testAndroidLog

Review of attachment 8641241 [details] [diff] [review]:
-----------------------------------------------------------------

rs=myk
Attachment #8641241 - Flags: review?(myk) → review+
Comment on attachment 8641815 [details] [diff] [review]
convert testWebChannel

Review of attachment 8641815 [details] [diff] [review]:
-----------------------------------------------------------------

I see no reason that this won't work, so r+.  However, I'd like to get a seasoned Desktop engineer to weigh in.  markh, would it be possible to test the toolkit (browser/ and mobile/android/) WebChannel implementation in mochitest-chrome (in toolkit/), thereby reducing test duplication?
Attachment #8641815 - Flags: review?(nalexander)
Attachment #8641815 - Flags: review+
Attachment #8641815 - Flags: feedback?(markh)
(In reply to Mark Finkle (:mfinkle) from comment #41)
> Kinda sad I re-used video_controls.html for this, but I guess that test
> might be converted at some point too :)

I'm glad you mentioned this. Since I'm not converting testVideoControls, I'd better copy video_controls.html instead of moving it (deleting the old file passes try because testVideoControls is disabled, but still...)
Comment on attachment 8641815 [details] [diff] [review]
convert testWebChannel

Review of attachment 8641815 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I don't see any reason the test couldn't be mochitest-chrome on desktop too which would avoid the duplication and be a "good thing"(tm) nor a reason it couldn't just be done as part of this bug (other than the obvious of the extra effort it might take)
Attachment #8641815 - Flags: feedback?(markh) → feedback+
Keywords: leave-open
Comment on attachment 8641240 [details] [diff] [review]
convert testAccounts

Review of attachment 8641240 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/tests/browser/chrome/test_accounts.html
@@ +26,5 @@
> +    // Only one account should exist.
> +    ok(!syncExists || !firefoxExists, "at least one account does not exist");
> +    is(anyExists, firefoxExists || syncExists, "sync/firefox account existence consistent with any existence");
> +
> +    // TODO: How can this be cleaned up?

Not easily.
Attachment #8641240 - Flags: review?(rnewman) → review+
Blocks: 1190964
(In reply to Mark Hammond [:markh] from comment #49)
> Yeah, I don't see any reason the test couldn't be mochitest-chrome on
> desktop too which would avoid the duplication and be a "good thing"(tm) nor
> a reason it couldn't just be done as part of this bug (other than the
> obvious of the extra effort it might take)

Thanks. Filed bug 1190964 for follow-up.
Attachment #8641799 - Flags: review?(margaret.leibovic) → review+
Attachment #8641803 - Flags: review?(margaret.leibovic) → review+
Attachment #8641806 - Flags: review?(margaret.leibovic) → review+
Keywords: leave-open
Duplicate of this bug: 1049022
Duplicate of this bug: 1160008
Duplicate of this bug: 1066087
Duplicate of this bug: 1188595
You need to log in before you can comment on or make changes to this bug.