Closed Bug 873473 Opened 11 years ago Closed 11 years ago

Clean up test repository by removing outdated workarounds where the specific bugs have been fixed

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox23 wontfix, firefox24 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox23 --- wontfix
firefox24 --- fixed

People

(Reporter: andrei, Assigned: jfrench)

References

(
URL
)

Details

(Whiteboard: [mentor=whimboo][lang=js][good first bug])

Attachments

(1 file, 2 obsolete files)

remove bug workarounds (default) rev1.2
12.54 KB, patch
whimboo
: review+
Details | Diff | Splinter Review 
The mozmill-tests repository contains a number of workarounds and chunks of commented code (usually with a referenced bug or FF version) which need to be clean up.

All of the above mentioned issues need to be investigated; if the dependency has been fixed, remove the workaround/de-comment code.

Examples of what needs removing:

::: lib/software-update.js
@@ +195,5 @@
> +   // if (this.activeUpdate.patchCount == 2) {
> +   //   var patch0URL = this.activeUpdate.getPatchAt(0).URL;
> +   //   var patch1URL = this.activeUpdate.getPatchAt(1).URL;
> +   //     Test that the update snippet created by releng doesn't have the same
> +   //     url for both patches (bug 514040).

@@ +511,5 @@
>     * @param {MozMillController} browserController
>     *        Mozmill controller of the browser window
>     */
>    openDialog: function softwareUpdate_openDialog(browserController) {
> +    // TODO: After Firefox 4 has been released and we do not have to test any

::: lib/tabs.js
@@ +478,5 @@
>  
>          // RTL-locales need to be treated separately
>          if (utils.getEntity(this.getDtds(), "locale.dir") == "rtl") {
> +          // Bug 537968
> +          // Workaround until bug 537968 has been fixed

@@ +485,4 @@
>            this._controller.doubleClick(tabStrip, 100, 3);
>          } else {
> +          // Bug 537968
> +          // Workaround until bug 537968 has been fixed

::: lib/tabview.js
@@ +524,5 @@
>          nodeCollector.queryNodes(".name");
>          break;
>        case "group_undoButton":
> +        // Bug 596504
> +        // No reference to the undo button

::: tests/functional/testTabbedBrowsing/testNewTab.js
@@ +69,5 @@
>    // Open a new tab and check that 'about:newtab' has been opened
>    tabBrowser.openTab(aEventType);
>  
> +  // Bug 716108
> +  // Remove once 716108 lands
Most likely a good candidate for Jonathan? :)
Whiteboard: [lang=js][good first bug] → [mentor=whimboo][lang=js][good first bug]
I'm in :)
Assignee: nobody → tojonmz
URL: http://hg.mozilla.org/qa/mozmill-tests/
Cross referencing the bug from which this bug originated, bug 872918.
Ok at this current snapshot in time, out of 49 files in the repo containing a bug reference of some kind, we have 18 of those files, containing a total of 27 resolved bugs. The bugs referenced below are either resolved fixed, resolved worksforme, or a similar status.

The bug numbers and affected files are:

(727842)
restartTests/testAddons_installUninstallHardBlocklistedExtension/test2.js
(474486) functional/testBookmarks/testAddBookmarkToMenu.js
(708491,725486) functional/testSecurity/manifest.ini
(624027,578162) functional/testTabbedBrowsing/testBackgroundTabScrolling.js
(716108) functional/testTabbedBrowsing/testNewTab.js
(614579) l10n/testCropped/manifest.ini
(614579) l10n/testCropped/test1.js
(664018,658369,657492) remote/restartTests/manifest.ini
(664018) remote/restartTests/testDiscoveryPane_FirstTimeModule/test1.js
(658369,678478,680045) 
remote/restartTests/testDiscoveryPane_installAddonWithEULA/test1.js
(657492,666530) 
tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js
(599702,599775) lib/addons.js
(614949) lib/dom-utils.js
(661408) lib/modal-dialog.js
(555938) lib/search.js
(514040) lib/software-update.js
(537968) lib/tabs.js
(596504,648294) lib/tabview.js

Given that the repo evolves by the day, by the time we land this clean up work, the repo may have changed again. An obvious split-off bug will be to build some little script that drills through the repo for all lines containing a bug number, and then takes that sorted list of bug numbers and feeds it to a bugzilla command-line query for particular bug status(s). This would allow us to flag any files in this state in a once-a-week automated report, or similar.

I will begin the clean up work above and may ask questions about a few as I go along if code blocks to be re-exposed are ambiguous.
Making progress on the above. I will be providing an overview of test results when I have done a first pass (comparing existing repo vs. cleanup), so we can see where we stand on each file, and before considering building any patches.

I'm seeing many different errors with mozmill2.0rc3, so will be providing accompanying test results with 2.0rc3, only for historical interest. The main harness results for evaluating changes will be provided from 1.5.21.
I'm about to try to add a very large comment with the preliminary investigation results. No code, just lots of notes for initial discussion. We'll see if Bugzilla accepts it.
Ok, here's some preliminary investigation results, before considering what files get patched for this bug, what may not, what might be split off into other bugs if they require more work. Please make a pot of tea, it's kind of long :)

Please refer to the mozmill1.5.21 results below, for their state, and consideration.

Mozmill2.0rc3 has only been provided for historical purposes. There are many what I believe to be unrelated failures in 2.0. I've yet to get 2.0 running with any stability yet in general. 

Feel free to do an inline reply if you have any thoughts on any file. It's my intent to build a patch only for any tests where "cleanup passes" and I've flagged ones I think ready for a patch, as "RE-ENABLE".

In the case of the libs, I was running tests which were loosely related to that lib (eg. testTabView/ testTabbedBrowsing/), or tests which specifically required the lib.

(727842) tests/functional/restartTests/testAddons_installUninstallHardBlocklistedExtension/test2.js
- cleanup fails in mozmill1.5.21
  it appears httpd still needs to be restarted in test2 with existing workaround code
- current repo version fails in mozmill2.0rc3
  "message": "AddonsManager_isAddonInstalled: Add-on has been specified. - got 'undefined'"

(474486) tests/functional/testBookmarks/testAddBookmarkToMenu.js
- this needs additional investigation so is unknown at this time

RE-ENABLE
(708491,725486) tests/functional/testSecurity/manifest.ini
 708491 testSecurityNotification (manifest re-enable only, no required code removal)
- current repo version passes in mozmill1.5.21 (when re-enabled)
- current repo version fails on assert of identity in mozmill2.0rc3
 725486 testSubmitUnencryptedInfoWarning (manifest re-enable only, no required code removal)
- current repo version passes in mozmill1.5.21 (when re-enabled)
- current repo version fails on controller.waitForPageLoad() in mozmill2.0rc3

RE-ENABLE
(624027,578162) tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
- cleanup passes in mozmill1.5.21
- current repo version fails on a controller.waitForPageLoad() in mozmill2.0

(716108) tests/functional/testTabbedBrowsing/testNewTab.js
- cleanup (removal of specific controller.waitForPageLoad() ) fails when removed in mozmill1.5.21
  (and presumably 2.0rc3 were it possible to run in it)
- current repo version fails on the first, unrelated controller.waitForPageLoad() in mozmill2.0rc3

RE-ENABLE
(614579) tests/l10n/testCropped/manifest.ini
(614579) tests/l10n/testCropped/test1.js
- cleanup passes in mozmill1.5.21
- current repo version passes in mozmill2.0rc3

(664018,658369,657492) tests/remote/restartTests/manifest.ini
-- see all related below --
(664018) tests/remote/restartTests/testDiscoveryPane_FirstTimeModule/test1.js
- cleanup (unskip) fails in both mozmill1.5.21 and mozmill2.0rc3
  test hangs and times out, in utils.js
    "message": "Category has been changed.",
    "fileName": "resource://mozmill/stdlib/utils.js",
    "name": "TimeoutError",
(658369,678478,680045) tests/remote/restartTests/testDiscoveryPane_installAddonWithEULA/test1.js
- cleanup (unskip 658369) fails in mozmill1.5.21 and mozmill2.0rc3
  fails on 680045 which is Resolved Wontfix (Add Elements to UI Map)
  678478 is also Resolved Wontfix
  given the test will always fail, does this test get rewritten, retired, or left as skip with bug numbers
(657492,666530) tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js
- cleanup (unskip 657492) fails in mozmill1.5.21 and mozmill2.0rc3
  the skip was claimed to be for running with Default Nightly
  however the Pick of the Month still isn't found when unskipped, with Nightly, or with Release either
  - interestingly, 657492's bug title is about implementing Pick of the Month, where the skip comment
  is about not being compatible (different bug titles for same number)
  666530 is a workaround which allows the test to get to the Pick of the Month panel
  it's marked fixed but perhaps that should be left in for now even if the test becomes runnable

(599702,599775) lib/addons.js
599702 is Resolved Wontfix
http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/addons.js#l306
Recommend splitting off a separate bug to update global warning attribute handling if applicable, in the lib
599775 is Resolved Fixed
http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/addons.js#l1180
Recommend splitting off a separate bug to update radio group handling, in the lib

(614949) lib/dom-utils.js
http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/dom-utils.js#l404
Recommend splitting off a separate bug to update finding access keys of all parent/child types, in the lib

(661408) lib/modal-dialog.js
http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/modal-dialog.js#l95
Can we really remove the if isLoaded else if documentLoaded block here, based on the Resolved Fixed bug 661408 thread.. I'm not totally sure. As the bug comments have a bunch of missing github commit links so don't have a lot of context.

RE-ENABLE
(555938) lib/search.js
http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/search.js#l582
- cleanup passes in mozmill1.5.21
Ran functional/testSearch in which testOpenSearchAutodiscovery calls case of the affected method and it passes
- current repo version fails sporadically in mozmill2.0rc3 on testAddMozSearchProvider
"message": ": could not find element Name: add",

RE-ENABLE
(514040) lib/software-update.js
http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/software-update.js#l179
- cleanup 'passes' in mozmill1.5.21 (same number of update tests pass before/after change)
However isCompleteUpdate does not appear to be called anywhere in the mozmill repo at present, only defined in this lib
- current repo fails with mozmill2.0rc3
(testDirectUpdate)
"message": "controller(): Window could not be initialized."
(testFallbackUpdate)
"message": "The update channel has been set correctly. - 'undefined' should equal 'nightly'"

RE-ENABLE
(537968) lib/tabs.js
http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/tabs.js#l483
- cleanup passes in mozmill1.5.21
- current repo version fails in mozmill2.0rc3
"message": "controller.waitForPageLoad(): Timeout waiting for page loaded."

RE-ENABLE
(596504,648294) lib/tabview.js
596504
http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/tabview.js#l512
- cleanup 'passes' in mozmill1.5.21 (tests which require tabview consistent before/after change)
648294
http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/tabview.js#l121
- cleanup 'passes' in mozmill1.5.21 (tests which require tabview consistent before/after change)
- unclear if the above 2 code paths are actually being called by any existing tests
(nb. tests that require tabview lib)
tests/endurance/testTabView_OpenNewTab/test1.js
tests/endurance/testTabView_OpenTabViewWithTabs/test1.js
tests/endurance/testTabView_SwitchTabs/test1.js
tests/functional/testTabView/testTabGroupNaming.js
tests/functional/testTabView/testToggleTabView.js
lib/tests/testTabView.js
(In reply to Jonathan French (:jfrench) from comment #7)
> (727842)
> tests/functional/restartTests/
> testAddons_installUninstallHardBlocklistedExtension/test2.js
> - cleanup fails in mozmill1.5.21
>   it appears httpd still needs to be restarted in test2 with existing
> workaround code
> - current repo version fails in mozmill2.0rc3
>   "message": "AddonsManager_isAddonInstalled: Add-on has been specified. -
> got 'undefined'"

We have a workaround for now and we will live with that. For the future we will abandon httpd.js in favor of mozhttpd. Then this problem will go away.

> RE-ENABLE
> (708491,725486) tests/functional/testSecurity/manifest.ini
>  708491 testSecurityNotification (manifest re-enable only, no required code
> removal)
> - current repo version passes in mozmill1.5.21 (when re-enabled)
> - current repo version fails on assert of identity in mozmill2.0rc3

Any test with a lookup expression might fail in 2.0rc3. So there is no need to check that at this point.

> RE-ENABLE
> (624027,578162)
> tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
> - cleanup passes in mozmill1.5.21
> - current repo version fails on a controller.waitForPageLoad() in mozmill2.0

Might be interesting to investigate that later why it's failing in 2.0.

> (716108) tests/functional/testTabbedBrowsing/testNewTab.js
> - cleanup (removal of specific controller.waitForPageLoad() ) fails when
> removed in mozmill1.5.21
>   (and presumably 2.0rc3 were it possible to run in it)
> - current repo version fails on the first, unrelated
> controller.waitForPageLoad() in mozmill2.0rc3

In mozmill 2.0 we can call waitForPageLoad only once for a page load while with 1.5 you could call it multiple times.

> (664018,658369,657492) tests/remote/restartTests/manifest.ini
> -- see all related below --
> (664018) tests/remote/restartTests/testDiscoveryPane_FirstTimeModule/test1.js
> - cleanup (unskip) fails in both mozmill1.5.21 and mozmill2.0rc3
>   test hangs and times out, in utils.js
>     "message": "Category has been changed.",
>     "fileName": "resource://mozmill/stdlib/utils.js",
>     "name": "TimeoutError",

For all discovery pane tests please check bug 732353. Most likely we can skip those for now.

> (599702,599775) lib/addons.js
> 599702 is Resolved Wontfix
> http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/addons.js#l306
> Recommend splitting off a separate bug to update global warning attribute
> handling if applicable, in the lib

Sounds fine to me, and for all the other requests which need code to be changed. This bug should only cover the removal of those comments and updates of commented out code.

> (661408) lib/modal-dialog.js
> http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/modal-dialog.
> js#l95
> Can we really remove the if isLoaded else if documentLoaded block here,
> based on the Resolved Fixed bug 661408 thread.. I'm not totally sure. As the
> bug comments have a bunch of missing github commit links so don't have a lot
> of context.

Both attributes don't exist anymore. From now on window.isLoaded() has to be used.

> RE-ENABLE
> (514040) lib/software-update.js
> http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/software-update.
> js#l179
> - cleanup 'passes' in mozmill1.5.21 (same number of update tests pass
> before/after change)
> However isCompleteUpdate does not appear to be called anywhere in the
> mozmill repo at present, only defined in this lib

No, it's used in patchInfo() in the same lib.

> - current repo fails with mozmill2.0rc3
> (testDirectUpdate)
> "message": "controller(): Window could not be initialized."
> (testFallbackUpdate)
> "message": "The update channel has been set correctly. - 'undefined' should
> equal 'nightly'"

You will have to run the test through the testrun_update.py script. Otherwise it will fail, that's correct. Get the script from here: https://github.com/whimboo/mozmill-automation

> RE-ENABLE
> (537968) lib/tabs.js
> http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/tabs.js#l483
> - cleanup passes in mozmill1.5.21
> - current repo version fails in mozmill2.0rc3
> "message": "controller.waitForPageLoad(): Timeout waiting for page loaded."

Most likely something we should really investigate for 2.0
Thanks, I will build a patch for this bug based on that info. I will split off separate bugs for the above issues after landing.
Here are the files containing resolved bug numbers, but will not be part of the patch, with their reasons. Files marked with NEW-BUG, require additional new code to be cleaned up, and so were omitted as per request and split off into a new bug. See Comment 7 where required for context.

Other issues specific to mozmill2.0 referenced in Comment 8 were investigated in a separate thread.

LEAVE
functional/restartTests/testAddons_installUninstallHardBlocklistedExtension/test2.js

LEAVE, NEW-BUG(bug 875451)
functional/testBookmarks/testAddBookmarkToMenu.js

LEAVE
remote/restartTests/manifest.ini
remote/restartTests/testDiscoveryPane_FirstTimeModule/test1.js
remote/restartTests/testDiscoveryPane_installAddonWithEULA/test1.js
remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js
+ all appear they will be un-Skipped in Daniela's v4.0 patch in bug 732353

LEAVE, NEW-BUGS (bug 875773, bug 875805)
lib/addons.js

LEAVE, NEW-BUG (bug 875919)
lib/dom-utils.js
(In reply to Henrik Skupin (:whimboo) from comment #8)
> 
> > (661408) lib/modal-dialog.js
> > http://hg.mozilla.org/qa/mozmill-tests/file/21a9424d5762/lib/modal-dialog.js#l86

// XXX: bug 661408
// Remove extra checks once Mozmill 1.5.4 has been released
if ("isLoaded" in controller)
  isLoaded = controller.isLoaded()
else if ("documentLoaded" in controller.window)
  isLoaded = controller.window.documentLoaded;

> Both attributes don't exist anymore. From now on window.isLoaded() has to be
> used.

Questions about this above:

a) is the use of window.isLoaded() supported by mozmill1.5.21?
b) does that reference to mozmill "1.5.4" really mean mozmill2.0 now?
c) controller.isLoaded() appears to still be needed in mozmill 1.5.21 - ie. when I switch it to window.isLoaded(), tests using modal dialogs like testFormManager/testClearFormHistory will fail (it will hang not getting a true condition that the modal dialog is open, which it is)

If I take the code-comment literally - is it too early to change the module? 

I'm unclear what action to take on this code block. I could open a new marker bug for this change if it's outside the scope of this bug.
(In reply to Jonathan French (:jfrench) from comment #11)
> a) is the use of window.isLoaded() supported by mozmill1.5.21?

This is controller.isLoaded().

> b) does that reference to mozmill "1.5.4" really mean mozmill2.0 now?

As you can see in the referenced bug 661408 it has been landed for 1.5.4 a long time ago.

> c) controller.isLoaded() appears to still be needed in mozmill 1.5.21 - ie.
> when I switch it to window.isLoaded(), tests using modal dialogs like
> testFormManager/testClearFormHistory will fail (it will hang not getting a
> true condition that the modal dialog is open, which it is)

Well, see above. Not sure what the 'window' object is like in your case.

> I'm unclear what action to take on this code block. I could open a new
> marker bug for this change if it's outside the scope of this bug.

Yes, change it. We no longer use the else clause.
(In reply to Henrik Skupin (:whimboo) from comment #12)
> > b) does that reference to mozmill "1.5.4" really mean mozmill2.0 now?
> 
> As you can see in the referenced bug 661408 it has been landed for 1.5.4 a
> long time ago.

Yes, that mozmill versioning was part of my confusion. That bug makes reference to 1.5.4 in 2011-07-05 when it was resolved fixed, but the latest version available presently in 2013 via http://mozqa.com/mozmill-env/ is 1.5.21. Ie. it precedes it.
(In reply to Jonathan French (:jfrench) from comment #13)
> 
> Yes, that mozmill versioning was part of my confusion. That bug makes
> reference to 1.5.4 in 2011-07-05 when it was resolved fixed, but the latest
> version available presently in 2013 via http://mozqa.com/mozmill-env/ is
> 1.5.21. Ie. it precedes it.

Or at least I interpreted the 1.5.x numbering sequence that way for some reason (that 4 was a minor revision number, ahead of 2..21). Which is evidently not the case.
Patch "remove bug workarounds (default) rev1.0" for Default branch. Ten files modified.

This represents work to remove all resolved bug workarounds from the repo.

Retested patch vs. clean default with all mozmill-automation tests locally, making sure results were consistent. Then, also sent a full set of passing mozmill-automation test runs to Crowd, using Nightly 20130531031002. See [1].

All tests + patch, are the same results as default, with that build ID.

Overview of files changed:

RE-ENABLED
tests/functional/testSecurity/manifest.ini
- testSecurityNotification, testSubmitUnencryptedInfoWarning had been correctly un-skipped
- manifest should just have been updated when bug was resolved

CHANGED
tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
- sleep should have been removed when bug was resolved

CHANGED
tests/functional/testTabbedBrowsing/testNewTab.js
- sleep() should have been removed when bug resolved but test will still fail when removed
- best we can do for now is still refer to the resolved bug to have history of it

RE-ENABLE
tests/l10n/testCropped/manifest.ini
tests/l10n/testCropped/test1.js
- should have been unskipped when bug was resolved
- note this enables 1 new test and 2 new passes, with crowd runs

CHANGED
lib/modal-dialog.js
- lib should have been updated when bug was resolved
- unused documentLoaded else statement removed

CHANGED
lib/search.js
- lib should have been updated when bug was resolved
- normal method for cases engine,engine-manager, re-enabled

PURGED
lib/software-update.js
- unused code purged, bug was already resolved

PURGED
lib/tabs.js
- tabstrip workaround removed, bug was already resolved

CHANGED
lib/tabview.js
- lowercase now supported in DTD file
- nodeCollector.queryNodes(".undo") now supported


[1] Windows Crowd Runs
Functional
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b05623929168b4316
Endurance
http://mozmill-crowd.blargon7.com/#/endurance/report/c93c51b0c0b1af1b05623929168b4a88
Add-ons
http://mozmill-crowd.blargon7.com/#/addons/report/c93c51b0c0b1af1b05623929168b4cb3
L10n
http://mozmill-crowd.blargon7.com/#/l10n/report/c93c51b0c0b1af1b05623929168b4d23
Remote
http://mozmill-crowd.blargon7.com/#/remote/report/c93c51b0c0b1af1b05623929168b611a

Update had been previously tested.
Attachment #756999 - Flags: review?(hskupin)
Attachment #756999 - Flags: review?(dave.hunt)
Comment on attachment 756999 [details] [diff] [review]
remove bug workarounds (default) rev1.0

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

Lots of nice removals here. I haven't tested this patch yet, just made a review so that the code looks like what we want to get landed. I will run a test with the next version of the patch. Thanks Jonathan!

::: lib/modal-dialog.js
@@ -98,1 @@
>          if ("isLoaded" in controller)

You can also remove the if statement. It's not necessary anymore.

::: lib/software-update.js
@@ -197,5 @@
> -//      var patch1URL = this.activeUpdate.getPatchAt(1).URL;
> -       // Test that the update snippet created by releng doesn't have the same
> -       // url for both patches (bug 514040).
> -//      assert.notEqual(patch0URL, patch1URL, "Update snippets have different URLs");
> -//    }

I don't think we want to remove this code. :) That's important and checks that the partial patch is different from the complete one. We might want to remove the extra comment and also update the notEqual() message for what we are testing.

::: tests/functional/testSecurity/manifest.ini
@@ -14,2 @@
>  [testSubmitUnencryptedInfoWarning.js]
> -disabled = Bug 725486 - Failure in testSecurity/testSubmitUnencryptedInfoWarning.js | The value in the search field should equal 'mozilla'

Those are disabled because they were failing with Mozmill 2.0. Are those fixed now?

::: tests/l10n/testCropped/manifest.ini
@@ -1,2 @@
>  [test1.js]
> -disabled = Bug 614579 - Crop test sometimes shows single line for cropped elements

We cannot enable that test cause it's still broken. See my next comment.

::: tests/l10n/testCropped/test1.js
@@ -250,5 @@
>    this.removeAttribute("data-command-backup");
>  }
> -
> -// Bug 614579 - Crop test sometimes shows single line for cropped elements
> -setupModule.__force_skip__ = "Bug 614579 - Crop test sometimes shows single line for cropped elements";

We are still failing for those tests for some locales or/and platforms. I can't tell what all we should have to fix first. But you might want to have a look at http://mozmill-ci.blargon7.com/#/l10n/reports.
Attachment #756999 - Flags: review?(hskupin)
Attachment #756999 - Flags: review?(dave.hunt)
Attachment #756999 - Flags: review-
(In reply to Henrik Skupin (:whimboo) from comment #16)
> Comment on attachment 756999 [details] [diff] [review]
> ::: lib/modal-dialog.js
> @@ -98,1 @@
> >          if ("isLoaded" in controller)
> 
> You can also remove the if statement. It's not necessary anymore.

Many tests, for example thirteen tests in testPreferences/, fail and hang with the Options window stuck open, without this check above present in lib/modal-dialog. Since it appeared that portion of the check was used, I left it in.

eg.
testDefaultPhishingEnabled.js
testEnableCookies.js
testDisableCookies.js
testPaneRetention.js
testPasswordNotSaved.js
testPasswordSavedAndDeleted.js
testPreferredLanguage.js
testRemoveAllCookies.js
etc..


> ::: lib/software-update.js
> @@ -197,5 @@
> > -//      var patch1URL = this.activeUpdate.getPatchAt(1).URL;
> > -       // Test that the update snippet created by releng doesn't have the same
> > -       // url for both patches (bug 514040).
> > -//      assert.notEqual(patch0URL, patch1URL, "Update snippets have different URLs");
> > -//    }
> 
> I don't think we want to remove this code. :) That's important and checks
> that the partial patch is different from the complete one. We might want to
> remove the extra comment and also update the notEqual() message for what we
> are testing.

Yes, my understanding from the resolved bug 514040 was that this check is no longer required as the fix now only offers two updates (Partial and Complete) if they differ (ie. Partial and Complete will never be identical now.. in theory :) ). But if there was a regression, sure, I will leave the code in, uncomment it, and modify the comment and assert message.


> ::: tests/functional/testSecurity/manifest.ini
> @@ -14,2 @@
> >  [testSubmitUnencryptedInfoWarning.js]
> > -disabled = Bug 725486 - Failure in testSecurity/testSubmitUnencryptedInfoWarning.js | The value in the search field should equal 'mozilla'
> 
> Those are disabled because they were failing with Mozmill 2.0. Are those
> fixed now?

Yes, this test passes in mozmill 1.5.21 and mozmill 2.0rc3 latest, on my windows system, so was re-enabled. Also a test run of the entire manifest of testSecurity/ is consistent between both versions of mozmill also. They all pass, with one known Skip for testSSLDisabledErrorPage for both.


> ::: tests/l10n/testCropped/manifest.ini
> @@ -1,2 @@
> >  [test1.js]
> > -disabled = Bug 614579 - Crop test sometimes shows single line for cropped elements
> 
> We cannot enable that test cause it's still broken. See my next comment.
> 
> ::: tests/l10n/testCropped/test1.js
> @@ -250,5 @@
> >    this.removeAttribute("data-command-backup");
> >  }
> > -
> > -// Bug 614579 - Crop test sometimes shows single line for cropped elements
> > -setupModule.__force_skip__ = "Bug 614579 - Crop test sometimes shows single line for cropped elements";
> 
> We are still failing for those tests for some locales or/and platforms. I
> can't tell what all we should have to fix first. But you might want to have
> a look at http://mozmill-ci.blargon7.com/#/l10n/reports.

I will revert this in my patch, but should this bug get marked back to Assigned or something in that case? Or a new bug for the locales and I change the referenced bug in the test? Bug 614579 is presently marked Resolved Fixed, which would be incorrect if it's still failing.
(In reply to Jonathan French (:jfrench) from comment #17)
> > >          if ("isLoaded" in controller)
> > 
> > You can also remove the if statement. It's not necessary anymore.
> 
> Many tests, for example thirteen tests in testPreferences/, fail and hang
> with the Options window stuck open, without this check above present in
> lib/modal-dialog. Since it appeared that portion of the check was used, I
> left it in.
> 
> eg.
> testDefaultPhishingEnabled.js

Can you further check that? It should never fail given that isLoaded() is always present on the controller object. 

> > ::: lib/software-update.js
> > @@ -197,5 @@
> > > -//      var patch1URL = this.activeUpdate.getPatchAt(1).URL;
> > > -       // Test that the update snippet created by releng doesn't have the same
> > > -       // url for both patches (bug 514040).
> > > -//      assert.notEqual(patch0URL, patch1URL, "Update snippets have different URLs");
> > > -//    }
> > 
> > I don't think we want to remove this code. :) That's important and checks
> > that the partial patch is different from the complete one. We might want to
> > remove the extra comment and also update the notEqual() message for what we
> > are testing.
> 
> Yes, my understanding from the resolved bug 514040 was that this check is no
> longer required as the fix now only offers two updates (Partial and
> Complete) if they differ (ie. Partial and Complete will never be identical
> now.. in theory :) ). But if there was a regression, sure, I will leave the
> code in, uncomment it, and modify the comment and assert message.

We never ship a partial update which is identical to the complete one. In such a case only a complete update will be listed. This is a sanity check for how RelEng produces the patches and should be kept in, yes.

> > ::: tests/functional/testSecurity/manifest.ini
> > @@ -14,2 @@
> > >  [testSubmitUnencryptedInfoWarning.js]
> > > -disabled = Bug 725486 - Failure in testSecurity/testSubmitUnencryptedInfoWarning.js | The value in the search field should equal 'mozilla'
> > 
> > Those are disabled because they were failing with Mozmill 2.0. Are those
> > fixed now?
> 
> Yes, this test passes in mozmill 1.5.21 and mozmill 2.0rc3 latest, on my
> windows system, so was re-enabled. Also a test run of the entire manifest of
> testSecurity/ is consistent between both versions of mozmill also. They all
> pass, with one known Skip for testSSLDisabledErrorPage for both.

Would you mind to do a quick search which bug disabled this test? I highly suspect that it is still open and would need the skip patch backed-out. I wouldn't do it x-bug wise.

> > ::: tests/l10n/testCropped/test1.js
> > @@ -250,5 @@
> > >    this.removeAttribute("data-command-backup");
> > >  }
> > > -
> > > -// Bug 614579 - Crop test sometimes shows single line for cropped elements
> > > -setupModule.__force_skip__ = "Bug 614579 - Crop test sometimes shows single line for cropped elements";
> > 
> > We are still failing for those tests for some locales or/and platforms. I
> > can't tell what all we should have to fix first. But you might want to have
> > a look at http://mozmill-ci.blargon7.com/#/l10n/reports.
> 
> I will revert this in my patch, but should this bug get marked back to
> Assigned or something in that case? Or a new bug for the locales and I
> change the referenced bug in the test? Bug 614579 is presently marked
> Resolved Fixed, which would be incorrect if it's still failing.

You might want to check with an en-US build and some other locales. There are more open bugs for the cropped test. The one referenced here we might want to remove.
(In reply to Henrik Skupin (:whimboo) from comment #18)
> (In reply to Jonathan French (:jfrench) from comment #17)
> > > >          if ("isLoaded" in controller)
> > > 
> > > You can also remove the if statement. It's not necessary anymore.
> > 
> > Many tests, for example thirteen tests in testPreferences/, fail and hang
> > with the Options window stuck open, without this check above present in
> > lib/modal-dialog. Since it appeared that portion of the check was used, I
> > left it in.
> > 
> > eg.
> > testDefaultPhishingEnabled.js
> 
> Can you further check that? It should never fail given that isLoaded() is
> always present on the controller object.

If the if isLoaded was removed altogether, the callback immediately below in modal-dialog wouldn't be executed, no? I assume that is why that part needs to be present.
http://hg.mozilla.org/qa/mozmill-tests/file/6f85bc8d4ede/lib/modal-dialog.js#l107
  
> > > ::: tests/functional/testSecurity/manifest.ini
> > > @@ -14,2 @@
> > > >  [testSubmitUnencryptedInfoWarning.js]
> > > > -disabled = Bug 725486 - Failure in testSecurity/testSubmitUnencryptedInfoWarning.js | The value in the search field should equal 'mozilla'
> > > 
> > > Those are disabled because they were failing with Mozmill 2.0. Are those
> > > fixed now?
> > 
> > Yes, this test passes in mozmill 1.5.21 and mozmill 2.0rc3 latest, on my
> > windows system, so was re-enabled. Also a test run of the entire manifest of
> > testSecurity/ is consistent between both versions of mozmill also. They all
> > pass, with one known Skip for testSSLDisabledErrorPage for both.
> 
> Would you mind to do a quick search which bug disabled this test? I highly
> suspect that it is still open and would need the skip patch backed-out. I
> wouldn't do it x-bug wise.

If you mean which test is currently skipping testSSLDisabledErrorPage, it's bug 861521, and it's still correctly open and would only get unskipped when that bug gets fixed. My patch won't touch that file and or alter the manifest entry for that test.
 
> > > ::: tests/l10n/testCropped/test1.js
> > > @@ -250,5 @@
> > > >    this.removeAttribute("data-command-backup");
> > > >  }
> > > > -
> > > > -// Bug 614579 - Crop test sometimes shows single line for cropped elements
> > > > -setupModule.__force_skip__ = "Bug 614579 - Crop test sometimes shows single line for cropped elements";
> > > 
> > > We are still failing for those tests for some locales or/and platforms. I
> > > can't tell what all we should have to fix first. But you might want to have
> > > a look at http://mozmill-ci.blargon7.com/#/l10n/reports.
> > 
> > I will revert this in my patch, but should this bug get marked back to
> > Assigned or something in that case? Or a new bug for the locales and I
> > change the referenced bug in the test? Bug 614579 is presently marked
> > Resolved Fixed, which would be incorrect if it's still failing.
> 
> You might want to check with an en-US build and some other locales. There
> are more open bugs for the cropped test. The one referenced here we might
> want to remove.

So after some searching, it appears bug 763638 is the main crop test failure, and bug 741301 is a dependent bug skipping the crop test. The skip patch in 741301 didn't reference 741301 in the code change, it used a Resolved Fixed bug 614579 skip patch - hence the confusion. 

So there's several options here for associating the bugs. Perhaps we could make the original Resolved Fixed bug 614579, depend on the main crop test failure bug 763638? Or set 614579 as a SeeAlso bug in 763638,741301. That way there would at least be some connection between them and the code change. Or update the skip itself in the repo with the desired bug number, against 741301.

In either case, the 're-enabling' of testCropped/ will be reverted in my next patch rev, and it will remain disabled as per the above bugs.
(In reply to Jonathan French (:jfrench) from comment #19)
> If the if isLoaded was removed altogether, the callback immediately below in
> modal-dialog wouldn't be executed, no? I assume that is why that part needs
> to be present.
> http://hg.mozilla.org/qa/mozmill-tests/file/6f85bc8d4ede/lib/modal-dialog.
> js#l107

Not sure I understand, but you should only remove the if() statement, not the call to isLoaded(). We removed 'documentLoaded' in Mozmill 1.5.4 and replaced it with isLoaded().

> > Would you mind to do a quick search which bug disabled this test? I highly
> > suspect that it is still open and would need the skip patch backed-out. I
> > wouldn't do it x-bug wise.
> 
> If you mean which test is currently skipping testSSLDisabledErrorPage, it's
> bug 861521, and it's still correctly open and would only get unskipped when
> that bug gets fixed. My patch won't touch that file and or alter the
> manifest entry for that test.

Ok, so we should be fine. Thanks.

> > You might want to check with an en-US build and some other locales. There
> > are more open bugs for the cropped test. The one referenced here we might
> > want to remove.
> 
> So after some searching, it appears bug 763638 is the main crop test
> failure, and bug 741301 is a dependent bug skipping the crop test. The skip
> patch in 741301 didn't reference 741301 in the code change, it used a
> Resolved Fixed bug 614579 skip patch - hence the confusion. 
> 
> So there's several options here for associating the bugs. Perhaps we could
> make the original Resolved Fixed bug 614579, depend on the main crop test
> failure bug 763638? Or set 614579 as a SeeAlso bug in 763638,741301. That
> way there would at least be some connection between them and the code
> change. Or update the skip itself in the repo with the desired bug number,
> against 741301.
> 
> In either case, the 're-enabling' of testCropped/ will be reverted in my
> next patch rev, and it will remain disabled as per the above bugs.

I would only mention bug 741301 which is the real dependent bug on getting this test re-enabled.
> I would only mention bug 741301 which is the real dependent bug on getting this test re-enabled.

Sorry, do you mean here, update the skip code in the tests and manifest, with the bugnumber? Or one of the other options.
(In reply to Jonathan French (:jfrench) from comment #21)
> Sorry, do you mean here, update the skip code in the tests and manifest,
> with the bugnumber? Or one of the other options.

Exactly. An update for the bug number and text.
Status: NEW → ASSIGNED
Thanks Henrik, patch will be coming up shortly. Will be doing some local mozmill-automation runs first, just to re-check the patch.
Patch "remove bug workarounds (default) rev1.1" for Default branch. Ten files modified.

This represents work to remove all resolved bug workarounds from the repo.

This patch addresses changes discussed from Comment 16 through Comment 22.

l10n/testCropped has had its Skip remapped from bug 614579 to bug 741301. I will update that bug if desired when this patch is landed; so there is visibility the Skip number has changed intentionally.

Full local client run via mozmill-automation making sure results were consistent between clean default vs. patch.

Also re-tested select affected tests via -m manifest, for testSecurity, testTabbedBrowsing, plus all tests which use affected tab libs, with both mozmill 1.5.21 and mozmill 2.0 rc4, to again ensure default vs. patch were consistent within each version of mozmill.

Tested with Nightly 20130617031112. All tests pass where expected.
Attachment #756999 - Attachment is obsolete: true
Attachment #764353 - Flags: review?(hskupin)
Comment on attachment 764353 [details] [diff] [review]
remove bug workarounds (default) rev1.1

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

r+ with one nit addressed. Once done you can forward the r+. Also have you executed any tests? I would love to see some results for the functional and update tests reported to our crowd dashboard.

::: tests/l10n/testCropped/test1.js
@@ +250,5 @@
>    this.removeAttribute("data-command-backup");
>  }
>  
> +// Bug 741301
> +// Disable broken l10n crop tests until false positives have been resolved

nit: Remove those comments please. Those are not necessary given that all the information is in the skip lines already.
Attachment #764353 - Flags: review?(hskupin) → review+
(In reply to Henrik Skupin (:whimboo) from comment #25)
> Comment on attachment 764353 [details] [diff] [review]
> remove bug workarounds (default) rev1.1
> 
> Review of attachment 764353 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with one nit addressed. Once done you can forward the r+. Also have you
> executed any tests? I would love to see some results for the functional and
> update tests reported to our crowd dashboard.
> 

Thanks, will update that nit and provide a new patch. 

w.r.t. testing, yes pretty exhaustive testing locally, comparing the behaviour of default vs. patch, as per comments at the bottom of Comment 24. testrun addons, functional, endurance, l10n, remote. And a run of update with an earlier rev patch. Will send another run to crowd and include them in the comment.
Patch "remove bug workarounds (default) rev1.2" for Default branch. Ten files modified.

This represents work to remove all resolved bug workarounds from the repo.

This patch addresses the nit discussed in Comment 25.
Attachment #764353 - Attachment is obsolete: true
Attachment #766072 - Flags: review+
Attachment #766072 - Flags: review+ → review?(hskupin)
Comment on attachment 766072 [details] [diff] [review]
remove bug workarounds (default) rev1.2

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

Wonderful work, really! I love those simplifications, and good to see that some of the blockers are not blocking us anymore.

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/8b4b53bb5cc2
Attachment #766072 - Flags: review?(hskupin) → review+
Let this ride the train, so we can be sure to not regress something. Given that 24.0 will be the next ESR it will even be present on that branch. Thank you again Jonathan!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox23: --- → wontfix
status-firefox24: --- → fixed
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: