Created attachment 655209 [details] [diff] [review] work in progress The mozApps DOM API tests are aggressively refactored into shared functions that do most of the work of calling API methods and making assertions about their behavior. That makes it hard to understand and reason about the tests, even for someone who is familiar with the way they are written. But it's especially hard for developers who are unfamiliar with the code, and test code often needs to be understood and updated by such developers, f.e. when tests break unexpectedly. So these tests should be unrefactored for maximum readability. In particular, API calls to the APIs being tested, and assertions about the behaviors of those calls, should be inlined into each test file/function, so the sequence of calls and assertions in a given body of testing code is as obvious as possible. Here's a work in progress that does this unrefactoring (along with a few additional fixes to improve reliability). The patch is mostly complete, and tests pass, but I haven't yet merged in the fix for bug 785161, and there's some cleanup to do. This patch is best understood by reading the new versions of the files, as the patches change substantially all of the code. But it needs to be applied to a tree that does not have the fix for bug 785161 until I update the patch to incorporate those changes.
Created attachment 655832 [details] [diff] [review] patch v1: unrefactors tests Here's a version of the patch that should be ready for review. I'll describe it and ask for review just as soon as I make sure I didn't regress anything: https://tbpl.mozilla.org/?tree=Try&rev=a380b9015b39
Attachment #655209 - Attachment is obsolete: true
Created attachment 656271 [details] [diff] [review] patch v2: works around possible race condition on Mac Some Mac builds failed a couple of tests. I can't reproduce locally, but after reading through the code, I suspect a race condition in the native API that _isLaunchable uses to determine whether or not an app is installed on Mac, such that _isLaunchable sometimes returns false for apps that have just been installed. The current code sets allAppsLaunchable, which would work around such a problem, but the first version of my patch didn't do that. This version does. It also removes one more file that is no longer used and corrects an assertion message. https://tbpl.mozilla.org/?tree=Try&rev=fb66f040b4b5
Attachment #655832 - Attachment is obsolete: true
Created attachment 656659 [details] [diff] [review] patch v3: works around one more occurrence of race condition This version of the patch works around one more (hopefully the last) occurrence of that race condition on Mac, makes test_getNotInstalled.xul more robust against a spurious and misleading failure caused by the failure of another test (such as the race condition failure), and includes a few more minor readability improvements. https://tbpl.mozilla.org/?tree=Try&rev=e8c2bf7e107c
Attachment #656271 - Attachment is obsolete: true
Attachment #656659 - Flags: review?(fabrice)
Comment on attachment 656659 [details] [diff] [review] patch v3: works around one more occurrence of race condition Review of attachment 656659 [details] [diff] [review]: ----------------------------------------------------------------- \o/
Attachment #656659 - Flags: review?(fabrice) → review+
Created attachment 658232 [details] [diff] [review] patch v4: resolves trivial conflicts with recent commits This version resolves trivial conflicts with two recent commits that each modified a single test file: http://hg.mozilla.org/mozilla-central/rev/b4eaa2264afe - bug 786296 dom/tests/mochitest/webapps/test_install_app.xul http://hg.mozilla.org/mozilla-central/rev/030e483f5ec1 - bug 783573 dom/tests/mochitest/webapps/apphelper.js Carrying forward review, and I'm building clobber now to test locally, after which I'll do one last tryserver run before pushing the patch.
Local tests look good; sent to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=2b5b3a5041df
Created attachment 658528 [details] [diff] [review] patch v5: resolve one more trivial merge conflict This patch resolves one more trivial merge conflict caught by the tryserver run: a reference in the new test extensions/cookie/test/test_app_uninstall.html to the file dom/tests/mochitest/webapps/apps/super_crazy.webapp, which I replaced with basic.webapp: diff --git a/extensions/cookie/test/test_app_uninstall.html b/extensions/cookie/test/test_app_uninstall.html index 3f13934..18b3d58 100644 --- a/extensions/cookie/test/test_app_uninstall.html +++ b/extensions/cookie/test/test_app_uninstall.html @@ -73,7 +73,7 @@ SpecialPowers.setBoolPref('browser.mozApps.installer.dry_run', true); permManager.addFromPrincipal(window.document.nodePrincipal, "webapps-manage", Ci.nsIPermissionManager.ALLOW_ACTION); -var gManifestURL = "http://www.example.com:80/chrome/dom/tests/mochitest/webapps/apps/super_crazy.webapp"; +var gManifestURL = "http://www.example.com:80/chrome/dom/tests/mochitest/webapps/apps/basic.webapp"; confirmNextInstall(); That was the only issue in the tryserver results, and the obvious fix passes my local tests, so I'll push this to inbound.
Comment on attachment 658528 [details] [diff] [review] patch v5: resolve one more trivial merge conflict https://hg.mozilla.org/integration/mozilla-inbound/rev/57a0543b7f12
Attachment #658528 - Flags: checkin+
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.