Closed Bug 789916 Opened 13 years ago Closed 13 years ago

Get rid of controller.assertJSProperty() and controller.assertDOMProperty() in favor of expect.* and assert.* methods

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect

Tracking

(firefox18 fixed, firefox19 fixed, firefox20 fixed, firefox-esr10 wontfix, firefox-esr17 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed
firefox-esr10 --- wontfix
firefox-esr17 --- fixed

People

(Reporter: whimboo, Assigned: jfrench)

References

(
URL
)

Details

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

Attachments

(5 files, 15 obsolete files)

final patch
35.74 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
patchBug789916_backPortAuroraToBeta
35.37 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
follow-up for white-spaces (default)
11.08 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
follow-up for white-spaces (Beta)
11.04 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
initial-fix and white-spaces (Release)
37.01 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
Similar to bug 782918 we should update our tests and replace any instance of controller.assertJSProperty() with the appropriate assert.* or expect.* call.
Hi, I would like to work on this bug, could you please guide me on getting started with it ? Which are the files that need to be edited ? Please help me with that. Thanks
Hi Abhishek. Great to see that you are interested in working on that bug. So to be setup please read through the following doc how to setup Mozmill on your computer and how to get the tests and run those: https://developer.mozilla.org/en/Mozmill_Tests Once you did that you should search through the whole repository for usages of the assertJSProperty() method. All of those will have to be replaced by expect.* or assert.* methods. As mentioned in comment 0 please check bug 782918 how to implement checks with those new methods. There is one thing to obey. Expect.* are soft-assertions which let the test continue while assert.* stops it right away. So whenever we simply check for a property it should be possible to use expect. Only in rare cases we probably should make use of assert. If you have questions please join us on IRC in the #automation channel. We are always around to answer your questions. Should be easier as on Bugzilla.
Assignee: nobody → abhishekp.bugzilla
Status: NEW → ASSIGNED
Abhishek, would you mind to give us an update on how my last comment helped you in getting started? We kinda would like to see this bug fixed in the next two weeks. Would you be able to accomplish that?
Hi Henrik, I have installed the addon for mozmill in firefox and also installed mozmill in my ubuntu, I have downloaded the mozmill test repository as well. I wanted to know that is there any online facility where I can search all occurrences of controller.assertJSProperty() in the repository like we have for mozilla-central (http://mxr.mozilla.org/mozilla-central/) ? Sorry for the late response ! Thanks
No, there isn't but I think you can simply do this via grep on the command line or inside your editor's IDE. There shouldn't be that many occurrences.
Hi, I wanted to know in equal(aValue, aExpected, aMessage) method when will the aMessage be displayed, when test passes or when the test fails i.e when the aValue is equal to aExpected or unequal ? Also I have used the ok method for asserting for true boolean value in controller.assertJSProperty(allow, "disabled", true); as per our discussion on irc, but can I use the ok method if the value to assert is false ? as in controller.assertJSProperty(popDownAutoCompList, "popupOpen", false) or must I use equal() here ? Please help. Thanks.
(In reply to Abhishek Potnis [:abhishekp] from comment #6) > I wanted to know in equal(aValue, aExpected, aMessage) method when will the > aMessage be displayed, when test passes or when the test fails i.e when the > aValue is equal to aExpected or unequal ? The message will always be included whether if the test passes or fails. Therefore you have to use a wording which correlates to the topic we are testing, e.g. if an element should be disabled: "The update button is disabled". > Also I have used the ok method for asserting for true boolean value in > controller.assertJSProperty(allow, "disabled", true); > as per our discussion on irc, but can I use the ok method if the value to > assert is false ? > as in > controller.assertJSProperty(popDownAutoCompList, "popupOpen", false) > or must I use equal() here ? You can simply use `equal.ok(!popdownAutoCompList.popupOpen, "....")` which negates the expression.
Attached patch WIP v1 (obsolete) — — Splinter Review
Attachment #665935 - Flags: feedback?(hskupin)
Comment on attachment 665935 [details] [diff] [review] WIP v1 Review of attachment 665935 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch Abhishek. For the first iteration it looks good! There is nothing major to mention except some small nits you should obey when updating all the remaining tests. I'm looking forward to a complete patch with those changes. Again, good work! ::: tests/functional/restartTests/testDefaultBookmarks/test1.js @@ +63,5 @@ > return items.length == 2; > }, "Bookmarks Toolbar contains 2 items"); > > // Check if the Most Visited folder is visible and has the correct title > + expect.equal(items[0].label,toolbarNodes.getChild(0).title, "Most Visited Folder visible and has correct title"); You can remove the comment above which is now totally covered by the expect.equal() message. That will apply to all the changes across files. Beside that make sure to add a blank after each ','. Check our coding style for more details. ::: tests/functional/testFormManager/testAutoCompleteOff.js @@ +35,5 @@ > controller.sleep(500); > > // Verify source autocomplete=off > var popupAutoCompList = new elementslib.ID(controller.window.document, "PopupAutoComplete"); > + expect.ok(!popdownAutoCompList.popupOpen, "No Popups"); As message we should use "Auto-complete popup is not visible". ::: tests/functional/testFormManager/testClearFormHistory.js @@ +58,5 @@ > // Begin typing into the name fields and verify no popup > controller.waitForElement(firstName, TIMEOUT); > controller.type(firstName, FNAME.substring(0,2)); > controller.sleep(500); > + expect.ok(!popdownAutoCompList.popupOpen, "No Popups"); Same here and other replacements.
Attachment #665935 - Flags: feedback?(hskupin) → feedback+
Attached patch WIP v2 (obsolete) — — Splinter Review
Attachment #665935 - Attachment is obsolete: true
Attachment #668926 - Flags: feedback?(hskupin)
Comment on attachment 668926 [details] [diff] [review] WIP v2 Review of attachment 668926 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the late review. I was some days away. When checking the patch there is only one major glitch with the element properties retrieval. Other stuff are nits only. Looks good. So you can go ahead and include all the remaining tests into the patch. Thanks! ::: tests/functional/restartTests/testDefaultBookmarks/test1.js @@ +62,5 @@ > controller.assert(function() { > return items.length == 2; > }, "Bookmarks Toolbar contains 2 items"); > > + expect.equal(items[0].label, toolbarNodes.getChild(0).title, "Most Visited Folder visible and has correct title"); Please take the former comment 1:1. It's missing 'is' and 'the'. @@ +67,2 @@ > > + expect.equal(items[1].label, toolbarNodes.getChild(1).title, "Getting Started bookmarks title"); The message should say that the label of the Getting Started bookmark has been set correctly. ::: tests/functional/testFormManager/testAutoCompleteOff.js @@ +37,2 @@ > var popupAutoCompList = new elementslib.ID(controller.window.document, "PopupAutoComplete"); > + expect.ok(!popdownAutoCompList.popupOpen, "Auto-complete popup is not visible"); popdownAutoCompList is of type element, so you will have to call getNode() on it to get the real node which then has the popupOpen property set. This affects all of the checks where elements are involved. ::: tests/functional/testPreferences/testEnableCookies.js @@ +101,3 @@ > var removeCookieButton = new elementslib.ID(controller.window.document, "removeCookie"); > controller.waitForElement(removeCookieButton, TIMEOUT); > + expect.ok(!removeCookieButton.disabled, "A cookie is saved"); The message here should mention something about the remove button. ::: tests/functional/testPreferences/testPasswordNotSaved.js @@ -61,5 @@ > prefDialog.close(true); > } > > /** > - * Check that passwords haven't been saved Please do not remove a jsdoc comment which is part of a function head. Only comments in a test should be moved to the message parameter. ::: tests/functional/testSecurity/testGreenLarry.js @@ +49,2 @@ > var identityBox = new elementslib.ID(controller.window.document, "identity-box"); > + expect.equal(identityBox.className, "verifiedIdentity", "The identity box shows green"); We might want to have a better message here now that we displaying it in our results. What specifically is green here? @@ +57,5 @@ > controller.waitFor(function () { > return doorhanger.getNode().state === 'open'; > }, "Identity popup has been opened"); > > + expect.equal(doorhanger.className, "verifiedIdentity", "Larry UI is verified aka Green"); Same as above. ::: tests/functional/testSessionStore/testUndoTabFromContextMenu.js @@ +64,3 @@ > > // Restore recently closed tab via tab browser context menu' > controller.click(contextMenuItem); If the above expect.ok() fails we would fail here because we do not check internally yet if the target element is visible and can be clicked on. So I would propose you change those to assert instead.
Attachment #668926 - Flags: feedback?(hskupin) → feedback-
Abhishek, do you have any update for us? We would have to fix this bug soon, so I would appreciate if we can go on. If you are blocked by something please let me know. Thanks.
Very sorry Henrik for the unreasonable delay, will upload the patch by tomorrow or day after.
No worries. Thanks for the update thought! Looking forward.
I have covered all occurrences of controller.assertJSProperty(). Sorry I could not come with better messages as suggested by you in prev. review for var identityBox = new elementslib.ID(controller.window.document, "identity-box"); expect.equal(identityBox.className, "verifiedIdentity", "The identity box shows green"); and controller.waitFor(function () { return doorhanger.getNode().state === 'open'; }, "Identity popup has been opened"); expect.equal(doorhanger.className, "verifiedIdentity", "Larry UI is verified aka Green") Please suggest them to me. Thanks.
Attachment #668926 - Attachment is obsolete: true
Attachment #679116 - Flags: feedback?(hskupin)
Comment on attachment 679116 [details] [diff] [review] Replaced controller.assertJSProperty() in favor of expect.* and assert.* There are still a couple of files missing. Please run "grep -r 'assertJSProperty' ./" from within your mozmill-tests repository. The following instances I can find: ./lib/tests/testAddonsDiscoveryPane.js: discovery.controller.assertJSProperty(section, "id", "main-feature"); ./lib/tests/testAddonsManager.js: controller.assertJSProperty(addonsList, "localName", "richlistbox"); ./tests/functional/restartTests/testDefaultBookmarks/test1.js: controller.assertJSProperty(items[0], "label", toolbarNodes.getChild(0).title); ./tests/functional/restartTests/testDefaultBookmarks/test1.js: controller.assertJSProperty(items[1], "label", toolbarNodes.getChild(1).title); ./tests/functional/testFormManager/testAutoCompleteOff.js: controller.assertJSProperty(popupAutoCompList, "popupOpen", false); ./tests/functional/testFormManager/testClearFormHistory.js: controller.assertJSProperty(popDownAutoCompList, "popupOpen", false); ./tests/functional/testFormManager/testClearFormHistory.js: controller.assertJSProperty(popDownAutoCompList, "popupOpen", false); ./tests/functional/testPreferences/testDisableCookies.js: //controller.assertJSProperty(removeCookieButton, "disabled", true); ./tests/functional/testPreferences/testEnableCookies.js: controller.assertJSProperty(removeCookieButton, "disabled", false); ./tests/functional/testPreferences/testPasswordNotSaved.js: controller.assertJSProperty(removeLogin, 'disabled', 'true'); ./tests/functional/testPrivateBrowsing/testDisabledElements.js: controller.assertJSProperty(importEntry, "disabled", true); ./tests/functional/testPrivateBrowsing/testDisabledPermissions.js: controller.assertJSProperty(allow, "disabled", true); ./tests/functional/testSecurity/testDVCertificate.js: controller.assertJSProperty(identityBox, "className", "verifiedDomain"); ./tests/functional/testSecurity/testDVCertificate.js: controller.assertJSProperty(doorhanger, "className", "verifiedDomain"); ./tests/functional/testSecurity/testDVCertificate.js: controller.assertJSProperty(host, "textContent", gETLDService.getBaseDomainFromHost(cert.commonName)); ./tests/functional/testSecurity/testDVCertificate.js: controller.assertJSProperty(owner, "textContent", property); ./tests/functional/testSecurity/testDVCertificate.js: controller.assertJSProperty(verifier, "textContent", l10nVerifierLabel); ./tests/functional/testSecurity/testDVCertificate.js: controller.assertJSProperty(encryptionLabel, "textContent", l10nEncryptionLabel); ./tests/functional/testSecurity/testDVCertificate.js: controller.assertJSProperty(securityTab, "selected", "true"); ./tests/functional/testSecurity/testEncryptedPageWarning.js: controller.assertJSProperty(infoBody, "textContent", enterSecureMessage); ./tests/functional/testSecurity/testGreenLarry.js: controller.assertJSProperty(identityBox, "className", "verifiedIdentity"); ./tests/functional/testSecurity/testGreenLarry.js: controller.assertJSProperty(doorhanger, "className", "verifiedIdentity"); ./tests/functional/testSecurity/testGreenLarry.js: controller.assertJSProperty(host, "textContent", gETLDService.getBaseDomainFromHost(cert.commonName)); ./tests/functional/testSecurity/testGreenLarry.js: controller.assertJSProperty(owner, "textContent", cert.organization); ./tests/functional/testSecurity/testGreenLarry.js: controller.assertJSProperty(ownerLocation, "textContent", location); ./tests/functional/testSecurity/testGreenLarry.js: controller.assertJSProperty(verifier, "textContent", l10nVerifierLabel); ./tests/functional/testSecurity/testGreenLarry.js: controller.assertJSProperty(label, "textContent", l10nEncryptionLabel); ./tests/functional/testSecurity/testGreenLarry.js: controller.assertJSProperty(securityTab, "selected", "true"); ./tests/functional/testSecurity/testGreyLarry.js: controller.assertJSProperty(favicon, "hidden", false); ./tests/functional/testSecurity/testGreyLarry.js: controller.assertJSProperty(doorhanger, "className", "unknownIdentity"); ./tests/functional/testSecurity/testGreyLarry.js: controller.assertJSProperty(securityTab, "selected", "true"); ./tests/functional/testSecurity/testSecurityNotification.js: controller.assertJSProperty(identityBox, "className", "verifiedIdentity"); ./tests/functional/testSecurity/testSecurityNotification.js: controller.assertJSProperty(identityBox, "className", "unknownIdentity"); ./tests/functional/testSecurity/testSecurityNotification.js.orig: controller.assertJSProperty(identityBox, "className", "verifiedIdentity"); ./tests/functional/testSecurity/testSecurityNotification.js.orig: controller.assertJSProperty(identityBox, "className", "unknownIdentity"); ./tests/functional/testSecurity/testSubmitUnencryptedInfoWarning.js: controller.assertJSProperty(infoBody, "textContent", message); ./tests/functional/testSecurity/testUnknownIssuer.js: controller.assertJSProperty(link, "textContent", "secure.mur.at"); ./tests/functional/testSessionStore/testUndoTabFromContextMenu.js: controller.assertJSProperty(contextMenuItem, 'disabled', true); ./tests/functional/testSessionStore/testUndoTabFromContextMenu.js: controller.assertJSProperty(contextMenuItem, 'disabled', false); ./tests/functional/testSessionStore/testUndoTabFromContextMenu.js: controller.assertJSProperty(contextMenuItem, 'disabled', true); I kinda would like to give feedback/review on a patch which includes all those changes.
Attachment #679116 - Flags: feedback?(hskupin)
Blocks: 744007
I just noticed that we also have the corresponding assertDOMProperty() calls still present. We should include those too in this patch because both methods are tightly bound to each other. ./lib/tests/testAddonsManager.js: controller.assertDOMProperty(addon, "name", MOZMILL.name); ./lib/tests/testAddonsManager.js: controller.assertDOMProperty(addon, "type", "extension");
Summary: Get rid of controller.assertJSProperty() in favor of expect.* and assert.* methods → Get rid of controller.assertJSProperty() and controller.assertDOMProperty() in favor of expect.* and assert.* methods
Priority: -- → P2
Comment on attachment 679152 [details] [diff] [review] Replaced controller.assertJSProperty() and controller.assertDOMProperty() in favor of expect.* and assert.* Review of attachment 679152 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/tests/testAddonsDiscoveryPane.js @@ +23,5 @@ > expect.equal(discovery.getSections().length, 6, > "There have to be 6 different sections"); > > var section = discovery.getSection("main-feature"); > + discovery.expect.equal(section.id, "main-feature", "The main-feature section is selected"); expect is a global module so it can't be sticked under discovery. This should fail if you run the test. Did you miss that? Also it misses a getNode() to get the real node which is necessary here to compare with the value. ::: lib/tests/testAddonsManager.js @@ +32,5 @@ > var category = am.getCategoryById({id: "extension"}); > am.setCategory({category: category}); > > var addonsList = am.getElement({type: "addonsList"}); > + expect.equal(addonsList.localName, "richlistbox", "Rich List Box selected"); It misses getNode() too. This also applies to all upcoming changes. Please make sure you test it by actually running the tests. Given that I will drop checking further changes in that patch because it needs a massive update. I will come back once a new patch is available.
Attachment #679152 - Flags: feedback?(hskupin) → feedback-
I edited the testAddonsDiscoveryPane.js by making the line as expect.equal(section.getNode().id, "main-feature", "The main-feature section is selected"); but am getting a failure when I run the test, same for testAddonsManager.js where I changed the line to expect.equal(addonsList.getNode().localName, "richlistbox", "Rich List Box selected"); So I reverted the two files - testAddonsDiscoveryPane.js and testAddonsManager.js back to the original state and tested, then too the tests failed. I donot understand this. Please help with this. I have pastebin-ned the output of the tests here : http://pastebin.mozilla.org/1927950
The failures you see are different. I would suggest we fix those in another bug. To ensure your changes are fine start with the tests under /tests/functional.
All files that I have edited passed the test on testing them individually except the two - testAddonsDiscoveryPane.js and testAddonsManager.js The errors I am getting for the above 2 files are on the lines that I haven't edited.The errors I get are here: http://pastebin.mozilla.org/1936912 What should be done to get those 2 files fixed as well ? Thanks.
Attachment #679152 - Attachment is obsolete: true
Attachment #680599 - Flags: feedback?(hskupin)
Comment on attachment 680599 [details] [diff] [review] Replaced controller.assertJSProperty() and controller.assertDOMProperty() in favor of expect.* and assert.* Review of attachment 680599 [details] [diff] [review]: ----------------------------------------------------------------- In general it looks great now. Good work so far so you get my f+ on it! So I wouldn't worry about the failure of the two tests under lib. But you should also convert the assert(JS|DOM)Property calls, even when they are failing. It's not a regression introduced by your patch, so we can keep track of it in a separate bug. Otherwise I have a couple of comments on messages, which would have to be made clearer. ::: lib/tests/testAddonsDiscoveryPane.js @@ +23,5 @@ > expect.equal(discovery.getSections().length, 6, > "There have to be 6 different sections"); > > var section = discovery.getSection("main-feature"); > + expect.equal(section.getNode().id, "main-feature", "The main-feature section is selected"); Please try to avoid too long lines. Usually we expect a wrapping at 80. To realize that you can move the message to the next line, so that it starts right under 'section'. This applies to all of your changes in that patch. ::: lib/tests/testAddonsManager.js @@ +32,5 @@ > var category = am.getCategoryById({id: "extension"}); > am.setCategory({category: category}); > > var addonsList = am.getElement({type: "addonsList"}); > + expect.equal(addonsList.getNode().localName, "richlistbox", "Rich List Box selected"); 'richlistbox' is the node type. The message should simply mention 'Correct node type for list of add-ons'. @@ +38,2 @@ > var addon = am.getAddons({attribute: "value", value: MOZMILL.id})[0]; > + expect.equal(addon.getNode().getAttribute('name'), MOZMILL.name, "The name property of MOZMILL extension is checked"); I would use: 'Add-on has the correct name'. @@ +38,3 @@ > var addon = am.getAddons({attribute: "value", value: MOZMILL.id})[0]; > + expect.equal(addon.getNode().getAttribute('name'), MOZMILL.name, "The name property of MOZMILL extension is checked"); > + expect.equal(addon.getNode().getAttribute('type'), "extension", "The type property of MOZMILL extension is checked"); "Add-on is of type 'extension'". ::: tests/functional/restartTests/testDefaultBookmarks/test1.js @@ +61,5 @@ > // For a default profile there should be exactly 2 items > assert.equal(items.length, 2, "Bookmarks Toolbar contains 2 items"); > > + expect.equal(items[0].getNode().label, toolbarNodes.getChild(0).title, "The Most Visited folder is visible and has the correct title"); > + nit: There is a trailing space which should be removed. @@ +62,5 @@ > assert.equal(items.length, 2, "Bookmarks Toolbar contains 2 items"); > > + expect.equal(items[0].getNode().label, toolbarNodes.getChild(0).title, "The Most Visited folder is visible and has the correct title"); > + > + expect.equal(items[1].getNode().label, toolbarNodes.getChild(1).title, "The label of the Getting Started bookmark has been set correctly"); Use the same wording as above, just for a different type of bookmark. ::: tests/functional/testPreferences/testDisableCookies.js @@ +97,5 @@ > */ > function checkCookieNotSaved(controller) { > // XXX: Bug 513820 - Remove Cookies button is not cleared when cookie list is cleared > var removeCookieButton = new elementslib.ID(controller.window.document, "removeCookie"); > + //expect.ok(removeCookieButton.getNode().disabled, "Cookie Button is disabled"); "Remove Cookie button ...." ::: tests/functional/testPreferences/testEnableCookies.js @@ +101,3 @@ > var removeCookieButton = new elementslib.ID(controller.window.document, "removeCookie"); > controller.waitForElement(removeCookieButton, TIMEOUT); > + expect.ok(!removeCookieButton.getNode().disabled, "The Remove Cookie Button is disabled and a cookie is saved"); The second part of this message is checked below, so please delete it. ::: tests/functional/testPreferences/testPasswordNotSaved.js @@ +71,5 @@ > var filterField = new elementslib.ID(controller.window.document, "filter"); > controller.waitForElement(filterField, TIMEOUT); > > var removeLogin = new elementslib.ID(controller.window.document, "removeSignon"); > + expect.ok(removeLogin.getNode().disabled, "Passwords haven't been saved"); This tests the remove password button. So the message should be similar to the cookie one from above. ::: tests/functional/testPrivateBrowsing/testDisabledPermissions.js @@ +68,5 @@ > '/*[name()="menupopup"][5]' + > '/*[name()="menuitem"][1]'); > > controller.waitForElement(allow); > + expect.ok(allow.getNode().disabled, "Allow Popups is disabled"); Mind putting single quotes around 'Allow Popups'? ::: tests/functional/testSecurity/testDVCertificate.js @@ +38,2 @@ > var identityBox = new elementslib.ID(controller.window.document, "identity-box"); > + expect.equal(identityBox.getNode().className, "verifiedDomain", "Identity box shows green"); "Identity is verified" @@ +58,4 @@ > // XXX: Larry strips the 'www.' from the CName using the eTLDService > // This is expected behaviour for the time being (Bug 443116) > var host = new elementslib.ID(controller.window.document, "identity-popup-content-host"); > + expect.equal(host.getNode().textContent, gETLDService.getBaseDomainFromHost(cert.commonName), "The site identifier string is checked against the Cert"); Please wrap correctly and update the message so it reads: "The site identifier string is equal to the cert host". @@ +63,4 @@ > var owner = new elementslib.ID(controller.window.document, "identity-popup-content-owner"); > var property = utils.getProperty("chrome://browser/locale/browser.properties", > "identity.ownerUnknown2"); > + expect.equal(owner.getNode().textContent, property, "The owner identifier string, should be (unknown)"); Should read: "The owner identifier string is set" @@ +68,5 @@ > var l10nVerifierLabel = utils.getProperty("chrome://browser/locale/browser.properties", > "identity.identified.verifier"); > l10nVerifierLabel = l10nVerifierLabel.replace("%S", cert.issuerOrganization); > var verifier = new elementslib.ID(controller.window.document, "identity-popup-content-verifier"); > + expect.equal(verifier.getNode().textContent, l10nVerifierLabel, "The Verified by: %S string is checked"); " ... is set". @@ +74,4 @@ > var l10nEncryptionLabel = utils.getProperty("chrome://browser/locale/browser.properties", > "identity.encrypted"); > var encryptionLabel = new elementslib.ID(controller.window.document, "identity-popup-encryption-label"); > + expect.equal(encryptionLabel.getNode().textContent, l10nEncryptionLabel, "The Encryption Label text is checked"); "is set" @@ +92,2 @@ > var securityTab = new elementslib.ID(controller.window.document, "securityTab"); > + expect.ok(securityTab.getNode().selected, "The Security tab is selected by default"); This should be an assert because it's a hard requirement for the next tests, which would fail if the security tab wouldn't have been set. ::: tests/functional/testSecurity/testEncryptedPageWarning.js @@ +57,5 @@ > // Wait for the content to load > var infoBody = new elementslib.ID(controller.window.document, "info.body"); > controller.waitForElement(infoBody); > > + expect.equal(infoBody.getNode().textContent, enterSecureMessage, "The message text is verified"); "The dialog shows the security message" ::: tests/functional/testSecurity/testGreenLarry.js @@ +50,2 @@ > var identityBox = new elementslib.ID(controller.window.document, "identity-box"); > + expect.equal(identityBox.getNode().className, "verifiedIdentity", "The identity box shows green"); Same as for the DV test. Just remove it. @@ +70,4 @@ > // XXX: Larry strips the 'www.' from the CName using the eTLDService > // This is expected behaviour for the time being (bug 443116) > var host = new elementslib.ID(controller.window.document, "identity-popup-content-host"); > + expect.equal(host.getNode().textContent, gETLDService.getBaseDomainFromHost(cert.commonName), "The site identifier string conforms to the Cert"); Same as for DV: "... cert host". @@ +75,2 @@ > var owner = new elementslib.ID(controller.window.document, "identity-popup-content-owner"); > + expect.equal(owner.getNode().textContent, cert.organization, "Owner string conforms to the Cert"); "... cert organization" @@ +92,5 @@ > var comma = ','; > var location = city + '\n' + state + comma + ' ' + country; > var ownerLocation = new elementslib.ID(controller.window.document, > "identity-popup-content-supplemental"); > + expect.equal(ownerLocation.getNode().textContent, location, "Owner location string conforms to the Cert"); "... cert location" @@ +99,5 @@ > "identity.identified.verifier"); > l10nVerifierLabel = l10nVerifierLabel.replace("%S", cert.issuerOrganization); > var verifier = new elementslib.ID(controller.window.document, > "identity-popup-content-verifier"); > + expect.equal(verifier.getNode().textContent, l10nVerifierLabel, "the 'Verified by: %S' string is verified"); "... is set" @@ +105,5 @@ > var l10nEncryptionLabel = utils.getProperty("chrome://browser/locale/browser.properties", > "identity.encrypted"); > var label = new elementslib.ID(controller.window.document, > "identity-popup-encryption-label"); > + expect.equal(label.getNode().textContent, l10nEncryptionLabel, "Encryption Label text is verified"); "... is set" ::: tests/functional/testSecurity/testGreyLarry.js @@ +24,2 @@ > var favicon = new elementslib.ID(controller.window.document, "page-proxy-favicon"); > + expect.ok(!favicon.getNode().hidden, "The default grey (globe image) favicon is present"); The feature has been changed meanwhile so it should read: "The globe favicon is visible" ::: tests/functional/testSecurity/testSecurityNotification.js @@ +24,2 @@ > var identityBox = new elementslib.ID(controller.window.document, "identity-box"); > + expect.equal(identityBox.getNode().className, "verifiedIdentity", "Identity box should have a green background"); 'Identity is verified' @@ +27,5 @@ > // Go to an unsecure (HTTP) site > controller.open(UNSECURE_PAGE); > controller.waitForPageLoad(); > > + expect.equal(identityBox.getNode().className, "unknownIdentity", "Identity box should have a gray background"); 'Identity is unknown' ::: tests/functional/testSecurity/testSubmitUnencryptedInfoWarning.js @@ +90,5 @@ > // The message string contains "##" instead of \n for newlines. > // There are two instances in the string. Replace them both. > message = message.replace(/##/g, "\n\n"); > > + expect.equal(infoBody.getNode().textContent, message, "The message text is verified"); "The dialog shows the security message" ::: tests/functional/testSecurity/testUnknownIssuer.js @@ +24,3 @@ > var link = new elementslib.ID(controller.tabs.activeTab, "cert_domain_link"); > controller.waitForElement(link, gTimeout); > + expect.equal(link.getNode().textContent, "secure.mur.at", "The link in Technical Details is correct"); 'Domain name is visible'
Attachment #680599 - Flags: feedback?(hskupin) → feedback+
I have made the suggested changes, but did not understand this - @@ +62,5 @@ > assert.equal(items.length, 2, "Bookmarks Toolbar contains 2 items"); > > + expect.equal(items[0].getNode().label, toolbarNodes.getChild(0).title, "The Most Visited folder is visible and has the correct title"); > + > + expect.equal(items[1].getNode().label, toolbarNodes.getChild(1).title, "The label of the Getting Started bookmark has been set correctly"); Use the same wording as above, just for a different type of bookmark. Please help me with this. Thanks.
Attachment #680599 - Attachment is obsolete: true
Attachment #682994 - Flags: feedback?(hskupin)
Comment on attachment 682994 [details] [diff] [review] Replaced controller.assertJSProperty() and controller.assertDOMProperty() in favor of expect.* and assert.* Review of attachment 682994 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Abhishek Potnis [:abhishekp] from comment #24) > I have made the suggested changes, but did not understand this - > > @@ +62,5 @@ > > assert.equal(items.length, 2, "Bookmarks Toolbar contains 2 items"); > > > > + expect.equal(items[0].getNode().label, toolbarNodes.getChild(0).title, "The Most Visited folder is visible and has the correct title"); > > + > > + expect.equal(items[1].getNode().label, toolbarNodes.getChild(1).title, "The label of the Getting Started bookmark has been set correctly"); > > Use the same wording as above, just for a different type of bookmark. It's simply that the two checks I have commented on should have the same wording for the message. Those only differ in the bookmark details but that's all.
Attachment #682994 - Flags: feedback?(hskupin) → feedback+
Hi Abhishek Potnis. Would you mind doing the remaining simply update for the patch so we can get it landed? Thanks!
Missed a new line in the previous patch, so uploading the corrected one.
Attachment #687731 - Attachment is obsolete: true
Attachment #687731 - Flags: review?(hskupin)
Attachment #687736 - Flags: review?(hskupin)
Comment on attachment 687736 [details] [diff] [review] Replaced controller.assertJSProperty() and controller.assertDOMProperty() in favor of expect.* and assert.* Review of attachment 687736 [details] [diff] [review]: ----------------------------------------------------------------- All looks fine now but there is one change you will have to make. The test 'testSecurity/testEncryptedPageWarning.js' has been renamed to 'testMixedContentPage.js' on bug 810820. That means your patch has to be updated and checked against any other assert(JS/DOM)Property usage. Once this is done we are ready for check-in. Thanks a lot!
Attachment #687736 - Flags: review?(hskupin) → review-
Attached patch Updated patch (obsolete) — — Splinter Review
Attachment #687736 - Attachment is obsolete: true
Attachment #691312 - Flags: review?(hskupin)
Comment on attachment 691312 [details] [diff] [review] Updated patch Review of attachment 691312 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/testSecurity/testDVCertificate.js @@ +96,2 @@ > var securityTab = new elementslib.ID(controller.window.document, "securityTab"); > + assert.ok(securityTab.getNode().selected, "The Security tab is selected by default"); This change is causing a test failure. Looks like you missed to import assert correctly. Beside that all looks fine.
Attachment #691312 - Flags: review?(hskupin) → review-
Attached patch final patch — — Splinter Review
Attachment #691312 - Attachment is obsolete: true
Attachment #691826 - Flags: review?(hskupin)
Comment on attachment 691826 [details] [diff] [review] final patch Looks good now and I can't see a failure. So lets get this checked into the default branch. We will watch tomorrows results and if everything stays green I hope that you would be interested to backport this patch to our other branches called mozilla-aurora, mozilla-beta, mozilla-release, mozilla-esr17, and mozilla-esr10. While it sounds much it will not be. But some stuff would have to be changed given possible merge conflicts.
Attachment #691826 - Attachment description: Updated patch → final patch
Attachment #691826 - Flags: review?(hskupin) → review+
Sure, would love to help ! :)
Abhishek, can you please check if your patch applies cleanly onto the other named branches? We want to backport it to those as well. Thanks.
Hi Abhishek, do you have an update for us regarding the backports? TIA
Attached patch Patch for branch mozilla-aurora (obsolete) — — Splinter Review
I had edited the patch to apply cleanly for mozilla-aurora branch. While testing, I am getting a test failure on testDisableElements.js and testDisabledPermissions.js (http://pastebin.mozilla.org/2014397) All the other tests pass.
Attachment #695280 - Flags: review?(hskupin)
Comment on attachment 695280 [details] [diff] [review] Patch for branch mozilla-aurora Review of attachment 695280 [details] [diff] [review]: ----------------------------------------------------------------- Abhishek, your patch looks fine but sadly I cannot land it because nearly all files show whitespace issues at the beginning of the line. We do not support tabs but only spaces. Looks like you should update the preferences of your editor so it does not automatically insert tabs. Can you please update your patch? Also does it apply on beta/release/esr17/esr10? Thank you!
Attachment #695280 - Flags: review?(hskupin) → review-
Abhishek, will you have time to update the patch? I would appreciate that. Keep in mind that meanwhile the code has been merged from default to aurora. So you would have to check the beta branch to get this patch correctly applied. Thanks!
status-firefox17: affected → ---
status-firefox19: affected → fixed
Jonathan want to continue on the backports. Given that I haven't heard back from Abhishek for a long time I will change the assignee. Sorry that I have marked it as 19 fixed. That's not the case. The formerly patch for aurora should now get landed on the beta branch.
Assignee: abhishekp.bugzilla → tojonmz
status-firefox19: fixed → affected
I'm looking at the backport of the patch from aurora to beta, first.
Very sorry Henrik, I will not be able to work on this bug as I am very busy with my college and exams. Sorry, I should have informed you earlier.
I did a transplant in my local beta branch with the desired return (570edabfa273), and about 1/2 of the files transplanted cleanly, and the other 1/2 I had to resolve. Two issues. One minor, the other, perhaps can be addressed in another bug. Issue One: tests/functional/testSecurity/testGreyLarry.js // Check the favicon has no label this comment line existed in Beta branch up until this earlier change below - which removed it. However - that line _exists, in the current head of the Aurora branch http://hg.mozilla.org/qa/mozmill-tests/diff/b9d820445e89/tests/functional/testSecurity/testGreyLarry.js One would have thought this line shouldn't exist in Aurora since the patch was made after this return afaikt. My plan is to resolve this file for the Beta branch with this line in question removed. ie. continuing its current state, in Beta. Other issue: I did a careful run this morning of each test involved in the back port to Beta with Firefox Beta 19.0b2 latest. In brief, everything appears to be ok, except for one test. lib/tests/testAddonsDiscoveryPane.js The test is finding the Add On Discovery Pane successfully when the test is run. I also tried it in Aurora 20.0a2 also (which has the sanctioned returns) with the equivalent Aurora branch test, and it also failed there the same way. I am inclined to think the test has just fallen out of date with some recent content change on the Add On landing page, and the test will need to be debugged and updated in a separate bug in all branches, later. I searched but couldn't find any bug which is tracking the failure. I the meantime I am looking at correcting whitespace issues introduced into the files from the original patch, and converting any tabs to whitespace.
These were the files in the initial patch, and in my local transplant, that were/are impacted by tab-whitespace problems. I've corrected them in my local client, and re-tested with Beta 19.0b2 latest. They continue to pass as expected. The known failure is as described above. (fail) lib\tests\testAddonsDiscoveryPane.js (pass) lib\tests\testAddonsManager.js (pass) tests\functional\restartTests\testDefaultBookmarks\test1.js (pass) tests\functional\testSecurity\testDVCertificate.js (pass) tests\functional\testSecurity\testGreenLarry.js (pass) tests\functional\testSecurity\testGreyLarry.js
Initial attempt at back port patch for Aurora to Beta.
Attached patch patchBug789916_tabFix (obsolete) — — Splinter Review
Clean up unwanted tab-whitespace from 6 affected files, which were created in the original patch.
So I've built two patches and attached them to the bug. One patch for the Aurora to Beta transplant for Abhishek's original return (plus resolves), and the 2nd patch, for the tab-whitespace repair. First week with Mercurial, so lease let me know what I've done wrong if applicable :) For each, I did: hg export -o (file) changeset For reference, on my client: hg log -l 2 (tab whitespace fix from my patch queue) changeset: ac90020e245b branch: mozilla-beta tag: qbase tag: qtip tag: tabFixBug789916 tag: tip user: Jonathan French date: Wed Jan 23 19:44:03 2013 -0500 summary: Bug 789916 - Get rid of controller.assertJSProperty() and controller.assertDOMProperty() in favor of expect .* and assert.* methods. r=hskupin (the Aurora to Beta hg -transplant, and resolve) changeset: 0b6f7956c315 branch: mozilla-beta tag: qparent parent: 2888:03a7f6805077 user: Abhishek Potnis<abhishekp.bugzilla@gmail.com> date: Thu Dec 13 21:16:12 2012 +0530 summary: Bug 789916 - Replaced controller.assertJSProperty() and controller.assertDOMProperty() in favor of expect.* and assert.*. r=hskupin
(In reply to Jonathan French (:jfrench) from comment #44) > Issue One: > tests/functional/testSecurity/testGreyLarry.js > // Check the favicon has no label > this comment line existed in Beta branch up until this earlier change below > - which removed it. However - that line _exists, in the current head of the > Aurora branch > http://hg.mozilla.org/qa/mozmill-tests/diff/b9d820445e89/tests/functional/ > testSecurity/testGreyLarry.js > > One would have thought this line shouldn't exist in Aurora since the patch > was made after this return afaikt. My plan is to resolve this file for the > Beta branch with this line in question removed. ie. continuing its current > state, in Beta. Good catch. And that's true. The patch on bug 803088 missed to remove that for the default and aurora branch. There might also be other cases. If someone is eager enough we can fix it but it's nothing critical. > I did a careful run this morning of each test involved in the back port to > Beta with Firefox Beta 19.0b2 latest. In brief, everything appears to be ok, > except for one test. > > lib/tests/testAddonsDiscoveryPane.js > > The test is finding the Add On Discovery Pane successfully when the test is > run. I also tried it in Aurora 20.0a2 also (which has the sanctioned > returns) with the equivalent Aurora branch test, and it also failed there > the same way. I am inclined to think the test has just fallen out of date > with some recent content change on the Add On landing page, and the test > will need to be debugged and updated in a separate bug in all branches, > later. I searched but couldn't find any bug which is tracking the failure. Yes, don't worry about those checks. See comment 20 and comment 21 for it. We should get this fixed for sure. > I the meantime I am looking at correcting whitespace issues introduced into > the files from the original patch, and converting any tabs to whitespace. Thanks!
Comment on attachment 705671 [details] [diff] [review] patchBug789916_tabFix Review of attachment 705671 [details] [diff] [review]: ----------------------------------------------------------------- Would you mind to tell us the grep/sed command you have used to find those cases? It would be good for us to run the verification on it. ::: lib/tests/testAddonsDiscoveryPane.js @@ +24,5 @@ > "There have to be 6 different sections"); > > var section = discovery.getSection("main-feature"); > expect.equal(section.getNode().id, "main-feature", > + "The main-feature section is selected"); When we have to wrap parameters the next line would have to start after the opening bracket, means perfectly aligned with the first parameter. This applies to everything in that patch and would need an update.
Attachment #705671 - Flags: review-
Attachment #705669 - Flags: review?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #50) > Comment on attachment 705671 [details] [diff] [review] > patchBug789916_tabFix > > Review of attachment 705671 [details] [diff] [review]: > ----------------------------------------------------------------- > > Would you mind to tell us the grep/sed command you have used to find those > cases? It would be good for us to run the verification on it. I actually did some reviews in vi of all files involved with the original patch, setting ":set list" in vi, which exposes new lines, end of lines, and tab indents. Tab indents show up as ^I, in this method. I then carefully replaced each with whitespace, preserving the existing line indentation. I imagine there is a way you could sed the entire codebase, using regex to find any instances of tab. Better yet would be have some kind of hg pre-processor which would reject any patch containing a test file with tab in it (unless explicitly overridden with some -arg). This would prevent them getting into the code base to begin with. > > ::: lib/tests/testAddonsDiscoveryPane.js > @@ +24,5 @@ > > "There have to be 6 different sections"); > > > > var section = discovery.getSection("main-feature"); > > expect.equal(section.getNode().id, "main-feature", > > + "The main-feature section is selected"); > > When we have to wrap parameters the next line would have to start after the > opening bracket, means perfectly aligned with the first parameter. This > applies to everything in that patch and would need an update. Ok, I will build another new patch and review all files involved in the original patch and set any instances accordingly. I originally just "replaced" the tab indents with whitespace, preserving the original, earlier approved indentation of the lines (8 whitespace characters, +1). Based on what you are saying, I will indent these wrapped lines so the first quote " of the wrapped line, lines up 1 character after first opening "(" brace. eg. section "The main-feature section is selected"); I will create another patch for this for the Beta branch.
(In reply to Jonathan French (:jfrench) from comment #51) > find any instances of tab. Better yet would be have some kind of hg > pre-processor which would reject any patch containing a test file with tab > in it (unless explicitly overridden with some -arg). This would prevent Exactly. That's something we already considered but never had the time to get this setup. Meanwhile I think it would be really helpful. Same for line endings. Do you have experience with it? We could file a new bug for it.
(In reply to Henrik Skupin (:whimboo) from comment #52) > (In reply to Jonathan French (:jfrench) from comment #51) > > find any instances of tab. Better yet would be have some kind of hg > > pre-processor which would reject any patch containing a test file with tab > > in it (unless explicitly overridden with some -arg). This would prevent > > Exactly. That's something we already considered but never had the time to > get this setup. Meanwhile I think it would be really helpful. Same for line > endings. Do you have experience with it? We could file a new bug for it. I don't have any experience with it, but I could try poking around later to just get some kind of standalone shell prototype thing working. Ultimately, I would assume its implementation would actually be in Bugzilla itself, by a Bugzilla guru. So, if the user attempts to create an attachment for a Bug, _and_ that attachment is flagged as a Patch, the uploaded file is processed(scanned) for bad whitespace. And the upload is rejected with some suitable error listing the lines involved, if the processor encounters illegal characters in the patch file(s). I could envision some boolean tick in the Bugzilla attachment upload panel, which would be On by default. (x) Scan Patch for illegal white space (..or whatever you'd want to call it) This would give the user the ability to override (turn the boolean off), in the scenario some teams don't care about illegal whitespace, and/or just want a given patch uploaded. I tend to jump to final implementation, which is probably not always ideal, but that is my initial assumption about where the check should happen. If we know it's bad, we prevent it from getting attached in Bugzilla in the first place. I will update the bug with a new patch when I have the indents corrected.
I also updated the Mozmill Style Guide with an editor setting for tab-to-whitespace https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide
Ok, so I've prepared a new patch which includes the indent corrections requested, and all other white space rules stated in the Style Guide, addressed. I will attach it momentarily. Also confirmed no trailing white space, and now has a new line at the bottom of every file where needed. I've also re-tested the tests in the patch against Beta 19.0b2. modified in this upcoming Beta "indent" patch --------------------------------------------- lib/tests/testAddonsManager.js lib/tests/testAddonsDiscoveryPane.js tests/functional/restartTests/testDefaultBookmarks/test1.js tests/functional/testFormManager/testAutoCompleteOff.js tests/functional/testFormManager/testClearFormHistory.js tests/functional/testPreferences/testDisableCookies.js tests/functional/testPreferences/testEnableCookies.js tests/functional/testPrivateBrowsing/testDisabledElements.js tests/functional/testPrivateBrowsing/testDisabledPermissions.js tests/functional/testSecurity/testDVCertificate.js tests/functional/testSecurity/testGreenLarry.js tests/functional/testSecurity/testGreyLarry.js tests/functional/testSecurity/testSubmitUnencryptedInfoWarning.js tests/functional/testSecurity/testUnknownIssuer.js tests/functional/testSessionStore/testUndoTabFromContextMenu.js (n/a) tests/functional/testPreferences/testPasswordNotSaved.js (n/a) tests/functional/testSecurity/testSecurityNotification.js n/a - are files included in the earlier branch transplant patch - but not touched in the "tab" patch or this "indent" patch, since they contained no unwanted tabs or other issues.
Attached patch patchBug789915_indentFixBeta (obsolete) — — Splinter Review
Correct indents in affected files, which were created in the original patch. Also conform the tests to other style guide rules.
(In reply to Jonathan French (:jfrench) from comment #53) > I don't have any experience with it, but I could try poking around later to > just get some kind of standalone shell prototype thing working. Ultimately, > I would assume its implementation would actually be in Bugzilla itself, by a > Bugzilla guru. So, if the user attempts to create an attachment for a Bug, > _and_ that attachment is flagged as a Patch, the uploaded file is > processed(scanned) for bad whitespace. And the upload is rejected with some > suitable error listing the lines involved, if the processor encounters > illegal characters in the patch file(s). I could envision some boolean tick > in the Bugzilla attachment upload panel, which would be On by default. That wouldn't be helpful because the landing of the patches will happen directly from the local repository and not via Bugzilla. Also Bugzilla cannot say if tabs are correct or not. It's different between different languages and for e.g. make files. So we really would need a hook for the mozmill-tests repository. I can remember we discussed that once in the automation development meeting. I think it might worth a posting to our public mailing list so we can get this established finally.
Comment on attachment 706238 [details] [diff] [review] patchBug789915_indentFixBeta > Ok, so I've prepared a new patch which includes the indent corrections > requested, and all other white space rules stated in the Style Guide, > addressed. I will attach it momentarily. Please do not forget to set the review flag. Otherwise it could happen I don't see the review request right away.
Attachment #706238 - Flags: review?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #50) > Comment on attachment 705671 [details] [diff] [review] > patchBug789916_tabFix Do you also have an update for that patch for default and aurora?
Comment on attachment 706238 [details] [diff] [review] patchBug789915_indentFixBeta Review of attachment 706238 [details] [diff] [review]: ----------------------------------------------------------------- Please combine this with the other patch for beta. We should not checkin individual patches on older branches given that we know about the white-space issue now.
Attachment #706238 - Flags: review?(hskupin)
Comment on attachment 705669 [details] [diff] [review] patchBug789916_backPortAuroraToBeta Review of attachment 705669 [details] [diff] [review]: ----------------------------------------------------------------- Hm, looks like I have misread the indentation of the other patch on this bug. Looks like this is the patch for beta while the other white-space patch is for default and aurora. The patch name was just a bit misleading. This all looks good and passes: http://mozmill-sandbox.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51cf49b1 Pushed to beta: http://hg.mozilla.org/qa/mozmill-tests/rev/1d9847c97ae2
Attachment #705669 - Flags: review?(hskupin)
Attachment #705669 - Flags: review+
Attachment #705669 - Flags: checkin+
Jonathan, please check if the beta patch also applies to the other branches. If not and you have to update them, please make sure to update the user of the patch. So it will be you who is referenced. Thanks!
status-firefox19: affected → fixed
Comment on attachment 706238 [details] [diff] [review] patchBug789915_indentFixBeta Still not sure where this patch belongs to. Would be great if you can clarify. Thanks.
(In reply to Henrik Skupin (:whimboo) from comment #63) > Comment on attachment 706238 [details] [diff] [review] > patchBug789915_indentFixBeta > > Still not sure where this patch belongs to. Would be great if you can > clarify. Thanks. That patch belongs only to the branch identified in the name of the patch; Beta. It's my assumption these 3 patches need to be returned to Beta, to make that branch correct: patchBug789916_backPortAuroraToBeta patchBug789916_tabFix patchBug789915_indentFixBeta ...based on looking at the content of these patch files, and my newbie understanding of how mercurial represents these incremental, changes. Then when that branch is 'correct' with landed patches, I will sync to it, and do another transplant; this time from beta to release. If that is not the correct process, can you point me to the Mozmill docs which describes working synchronously on multiple branches before their related patches actually get landed. If I'm supposed to 'combine' all these 3 patches into a single patch, let me know and I will figure out how to do that. Agreed on the +,-,? flags for patch review. I looked at the legend in Bugzilla and it wasn't clear which one was appropriate, or any guidance on them in the Mozmill docs either; so I left them blank for this time, hoping which to use, +,-, would get clarified. Do I simply always set it to "-" when I want a review? Also, can you explain "update the user of the patch" is. I am guessing it is during the patch upload process to Bugzilla, I set it to be "Beta" or something. If that is what it is, will do that next time, for the release patch. I assume that you would rarely flag a patch with several files like this as applicable to earlier product branches, since files in the patch could have been deprecated in the "earlier" branch, no? I have to assume the patch would be invalid if that was the case. Or that certain features, or aspects of the test in one branch, may be invalid in the older branches. So each patch would need to be carefully evaluated and resolved against the older branch(s). If that's not the case, let me know.
Oh, yes and if there is some official naming conventions for patches before they are uploaded to Bugzilla, can you point me to the doc? I couldn't find any examples on the Mozmill docs. Just a reference to the hg export > patch, incantation. Thanks!
Fyi, I ran all the "Beta" versions of the tests, which represent the cumulative 3 Beta patches listed above, running Firefox 18.0.1/Release, and they all Pass (and fail/skip where expected).
And I just carefully diff'd all the files represented by these sequential Beta patches patchBug789916_backPortAuroraToBeta patchBug789916_tabFix patchBug789915_indentFixBeta ...against the current latest equivalent Release branch files; and it looks like these patches can be landed on the release branch with the desired results (no resolves or conflicts with other more recent changes). Separately - this file which was renamed according to Comment#29, still exists in the Release branch. It still contains assertJSProperty(). testSecurity/testEncryptedPageWarning.js
(In reply to Jonathan French (:jfrench) from comment #64) > > Also, can you explain "update the user of the patch" is. If you're saying to modify the user id of the change set to me when I build patch derived from a transplant; sure, I will see if I can do that. That was a by-product of the transplant; it inherits the user name from the original change set. eg. this transplant from Aurora to Beta changeset: 2892:0b6f7956c315 branch: mozilla-beta user: Abhishek Potnis<abhishekp.bugzilla@gmail.com> <--------*** date: Thu Dec 13 21:16:12 2012 +0530 summary: Bug 789916 - Replaced controller.assertJSProp If it's something else, feel free to let me know.
Ok, here's where we are at with a Beta to Release patch. Item's marked 'ok' I will be able to include in a Release patch with no other changes. Other items are marked as outstanding. Branch to Release --------------------------------- (ok) lib/tests/testAddonsManager.js (rename?) lib/tests/testAddonsDiscoveryPane.js (ok) tests/functional/restartTests/testDefaultBookmarks/test1.js (ok) tests/functional/testFormManager/testAutoCompleteOff.js (ok) tests/functional/testFormManager/testClearFormHistory.js (resolve) tests/functional/testPreferences/testDisableCookies.js (resolve) tests/functional/testPreferences/testEnableCookies.js (resolve) tests/functional/testPreferences/testPasswordNotSaved.js (ok) tests/functional/testPrivateBrowsing/testDisabledElements.js (ok) tests/functional/testPrivateBrowsing/testDisabledPermissions.js (resolve) tests/functional/testSecurity/testDVCertificate.js (resolve) tests/functional/testSecurity/testGreenLarry.js (ok) tests/functional/testSecurity/testGreyLarry.js (ok) tests/functional/testSecurity/testSecurityNotification.js (ok) tests/functional/testSecurity/testSubmitUnencryptedInfoWarning.js (ok) tests/functional/testSecurity/testUnknownIssuer.js (ok) tests/functional/testSessionStore/testUndoTabFromContextMenu.js Resolve - each has new(or old) code changes near the top of each test, present in Release branch versions; I need to figure out which is current or correct - the Beta tests run and pass however, with Release 18.0.1 Rename? - do we want testEncryptedPageWarning.js need to be renamed to testMixedContent.js in Release branch also?
Found the bug resulting in those differences (830688). Resolving the above files now to include those changes.
Ok, I have all the files above resolved for a Release patch. Ignore my "rename?" flag on testAddonDiscoveryPane.js in Comment#69. There were no issues with that file. The only outstanding question is what you'd like me to do with the existing testSecurity/testEncryptedPageWarning.js in the Release branch. If you would like the file renamed also like it is in the Beta branch, to testMixedContent.js? And would you like the test's content modified to be the same as Beta. Let me know and I will proceed from there.
Just checked bug 810820 for the name switch of testEncryptedPageWarning to testMixedContent and it says the bug was tagged as unaffected in Release/18.0. So I will create a Release patch as-is, with the existing work, so at least we can land these existing changes now to the Release Branch. Then will remove/fix the assertJSProperty() in testEncryptedPageWarning in the Release branch as a separate return once I hear back; if you guys want that.
Attached patch patchBug789916_Release_rev1_0 (obsolete) — — Splinter Review
First patch, for the Release branch.
Attachment #708608 - Flags: review?(hskupin)
Attachment #708608 - Flags: checkin?(hskupin)
Attached patch patchBug789916_Release_rev1_1 (obsolete) — — Splinter Review
Supplemental patch for Release branch. Fixes testEncryptedPageWarning.js, which is present only in the Release branch and hadn't been fixed. Tested locally with 18.0.1/Release.
Attachment #708733 - Flags: review?(hskupin)
Attachment #708733 - Flags: checkin?(hskupin)
I started preparing an esr17 patch, but in doing so, discovered that 4 tests are failing - both in esr17.0.2 Nightly, _and in Release with the patch applied: tests/functional/testPreferences/testDisableCookies.js tests/functional/testPreferences/testEnableCookies.js tests/functional/testSecurity/testDVCertificate.js tests/functional/testSecurity/testGreenLarry.js The failures are triggered by switching to expect/assert in Esr17 and Release, but don't appear to have anything to do with bad changes. In the sense, I've diffed all the files being tested, and they correct and consistent with the changes in Beta, and Aurora in which these 4 tests run fine. It appears in 3 of these 4 tests, the modules assumed to be present in other parts of the test harness, perhaps aren't present in Mozmill Release and Mozmill Esr17? Here are the errors: testDisableCookies - "cm is not defined" - line 103 testEnableCookies - "cm is not defined" - line 105 testDVCertificate - "gETLDService is not defined" - line 61 The fourth error is different testGreenLarry - "controller.waitForPageLoad():Timeout waiting for page loaded" So maybe these are someone else's fix for another outstanding bug. Let me know if you would like the Esr17 patch uploaded for this bug, I have it ready otherwise, and all the other tests are passing as expected.
These failures are likely related to bug 830688, however I don't see these failures in our CI. Perhaps you need to do an update and try running the tests again?
Ok, well that's good news. Yup, I did a pull/update of esr17 branch yesterday, just prior to integrating my changes locally. So my branch was up to date at the time. I'm running mozmill 1.5.20 on a windows XP32 SP3 client. I also tried 1.5.19 mozmill currently available in the mozilla-build distribution. If these 4 tests are running and passing for you on esr17.0.2 Nightly, and in Release, I'm going to upload the esr17 patch now for review.
Attached patch patchBug789916_Esr17_rev1_0 (obsolete) — — Splinter Review
Patch, for the esr17 branch.
Attachment #709474 - Flags: review?(hskupin)
Attachment #709474 - Flags: review?(dave.hunt)
Also, a reminder these two patches need to be propagated across beta, default, and aurora branches, based on syncs to those 3 branches I did this morning. They don't appear to be present yet in those branches. patchBug789916_tabFix patchBug789915_indentFixBeta
(In reply to Jonathan French (:jfrench) from comment #64) > ...based on looking at the content of these patch files, and my newbie > understanding of how mercurial represents these incremental, changes. Then > when that branch is 'correct' with landed patches, I will sync to it, and do > another transplant; this time from beta to release. If that is not the > correct process, can you point me to the Mozmill docs which describes > working synchronously on multiple branches before their related patches > actually get landed. What you simply do is to take the patch which is closest to the branch you want to work on and apply it via 'hg qimport' and 'hg qpush'. If merge failures happen you will have to solve those and run 'hg qref' to update the patch currently active in the queue. I wouldn't start with the 'hg transplant' command at this level. Also because it will not add an entry to the mq queue. > If I'm supposed to 'combine' all these 3 patches into a single patch, let me > know and I will figure out how to do that. I think that we can tackle the indentation fix in a follow-up across branches. I don't want that the code gets bitrotted and would need a larger overhaul. You already spent a lot of time on it, which I really appreciate. So I will get those remaining patches landed and you will come up with an indent fix for default first. Please do not attach the backports right away. > Agreed on the +,-,? flags for patch review. I looked at the legend in > Bugzilla and it wasn't clear which one was appropriate, or any guidance on > them in the Mozmill docs either; so I left them blank for this time, hoping > which to use, +,-, would get clarified. Do I simply always set it to "-" > when I want a review? No, + or - is getting set by the reviewer. You will set ? and specify my email address. That way I will get this patch added to my patch queue. > Also, can you explain "update the user of the patch" is. I am guessing it is > during the patch upload process to Bugzilla, I set it to be "Beta" or > something. If that is what it is, will do that next time, for the release > patch. When you run 'hg qref' you should at least update your username if it is not you yet. That's done via the -u option. If you have correctly setup your ~/.hgrc file it should be automatically done for new patches. > I have to assume the patch would be invalid if that was the case. Or that > certain features, or aspects of the test in one branch, may be invalid in > the older branches. So each patch would need to be carefully evaluated and > resolved against the older branch(s). If that's not the case, let me know. It's totally correct! And we always appreciate backports for changes like those because it will give us nice new features like bug 838192. (In reply to Jonathan French (:jfrench) from comment #65) > Oh, yes and if there is some official naming conventions for patches before > they are uploaded to Bugzilla, can you point me to the doc? I couldn't find > any examples on the Mozmill docs. Just a reference to the hg export > patch, > incantation. Thanks! No, there isn't. Something which is helpful if you would at least have the name of the branch in the name if such an additional patch is necessary and can't be simply transplanted. (In reply to Jonathan French (:jfrench) from comment #69) > Rename? > - do we want testEncryptedPageWarning.js need to be renamed to > testMixedContent.js in Release branch also? No, please keep the name as it is.
Comment on attachment 708733 [details] [diff] [review] patchBug789916_Release_rev1_1 Review of attachment 708733 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine but please combine it with the other patch for this particular branch.
Attachment #708733 - Flags: review?(hskupin)
Attachment #708733 - Flags: checkin?(hskupin)
Comment on attachment 708608 [details] [diff] [review] patchBug789916_Release_rev1_0 Review of attachment 708608 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/tests/testAddonsDiscoveryPane.js @@ +68,5 @@ > discovery.controller.click(button); > > am.close(); > } > + There is no additional full blank line necessary at the end of the file. Just leave it as is. Same applies to other files too. When you do this, can directly integrate the other indentation fixes? I think it would be better, even mentioned otherwise before. It shouldn't be that much work and we have less a hassle to backport even more code.
Attachment #708608 - Flags: review?(hskupin)
Attachment #708608 - Flags: review-
Attachment #708608 - Flags: checkin?(hskupin)
Comment on attachment 695280 [details] [diff] [review] Patch for branch mozilla-aurora A patch for aurora was never necessary given the merge at this time. Marking as obsolete.
Attachment #695280 - Attachment is obsolete: true
Comment on attachment 706238 [details] [diff] [review] patchBug789915_indentFixBeta Review of attachment 706238 [details] [diff] [review]: ----------------------------------------------------------------- Please get this merged with patchBug789916_tabFix for default, aurora, and beta.
Comment on attachment 709474 [details] [diff] [review] patchBug789916_Esr17_rev1_0 Resetting review for now until the release patch has been finalized and the indentation changes have been included.
Attachment #709474 - Flags: review?(hskupin)
Attachment #709474 - Flags: review?(dave.hunt)
(In reply to Henrik Skupin (:whimboo) from comment #82) > > am.close(); > > } > > + > > There is no additional full blank line necessary at the end of the file. > Just leave it as is. Same applies to other files too. Ok, but in that case the Mozmill Style Guide instructions on whitespace appear to be wrong: https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Whitespace "All files should end with a newline character" This also implies that every patch I've created for all branches (default, aurora, beta, release, esr17), is incorrect; as the patches all have newlines added in tests where they were not present. Which branch patch would you like re-created first, and I will start on that. Default?
No, there is a difference between 'newline character' and 'blank line'. Best would probably be to get the default, aurora, and beta branch cleaned-up. Then you can include those changes into the release and esr17 patch. How does that sound?
Ok I will start on default first. I will upload that patch when ready, and wait for it to be landed successfully. Then I will proceed to aurora.
(In reply to Henrik Skupin (:whimboo) from comment #87) > No, there is a difference between 'newline character' and 'blank line'. This should probably be clarified with an example in the Mozmill docs. If you can provide a clear differentiation, I will add it in. Many of the test files I've encountered have what you would describe as a 'blank line' in them, outside of the ones I edited.
(In reply to Jonathan French (:jfrench) from comment #89) > This should probably be clarified with an example in the Mozmill docs. If > you can provide a clear differentiation, I will add it in. Many of the test > files I've encountered have what you would describe as a 'blank line' in > them, outside of the ones I edited. None of our files should have an extra blank line at the end. Would you mind to reference such a file?
(In reply to Henrik Skupin (:whimboo) from comment #90) > (In reply to Jonathan French (:jfrench) from comment #89) > > This should probably be clarified with an example in the Mozmill docs. If > > you can provide a clear differentiation, I will add it in. Many of the test > > files I've encountered have what you would describe as a 'blank line' in > > them, outside of the ones I edited. > > None of our files should have an extra blank line at the end. Would you mind > to reference such a file? Sure, here's one as example - line 40 tests\functional\testSecurity\testMixedContentPage.js
Hm, interesting. That's indeed wrong. Thanks for catching that.
Patch, for the Default branch. Essentially, it consolidates the former tabFix patch and the indentFix patches - into a single patch to address these issues specific to the Default branch. Six files modified. Tested with Nightly 21.0a1 20130205031033. Tests pass where expected.
Attachment #705671 - Attachment is obsolete: true
Attachment #706238 - Attachment is obsolete: true
Attachment #710408 - Flags: review?(hskupin)
Comment on attachment 710408 [details] [diff] [review] follow-up for white-spaces (default) Review of attachment 710408 [details] [diff] [review]: ----------------------------------------------------------------- Great clean-up. That's how it should look like. Thanks Jonathan. As a further note regarding commit messages, please try to use a message which visualizes what your patch is actually doing. I will fix it this time and land the patch. Landed on default as: http://hg.mozilla.org/qa/mozmill-tests/rev/61adb8495cf3 Does it apply cleanly on aurora and beta so we can get this backported?
Attachment #710408 - Flags: review?(hskupin)
Attachment #710408 - Flags: review+
Attachment #710408 - Flags: checkin+
Attachment #710408 - Attachment description: patchBug789916_Default_rev1_0 → follow-up for white-spaces (default)
Great. No, I will be producing a new patch for each branch, after carefully reviewing and testing the changes for each. Once I've produced the next (aurora) you can review and land that. Then after you've landed aurora, I will do beta. Etc. It's conceivable the patch could end up the same; but each branch seems to be such a moving target, it seems safer this way. Thanks for the clarification of the commit message in the patch. Everyone else's changes I've looked at seemed to be repeating the bug title over and over again, which I thought was a bit odd, and non-descriptive. I will do what I normally would do (what you are suggesting).
(In reply to Jonathan French (:jfrench) from comment #95) > Great. No, I will be producing a new patch for each branch, after carefully > reviewing and testing the changes for each. Once I've produced the next > (aurora) you can review and land that. Then after you've landed aurora, I > will do beta. Etc. Please only attach a new patch if it differs from default. Same applies for the beta branch. If patches are identical we want to transplant them. > Thanks for the clarification of the commit message in the patch. Everyone > else's changes I've looked at seemed to be repeating the bug title over and > over again, which I thought was a bit odd, and non-descriptive. I will do > what I normally would do (what you are suggesting). Yes, that's something we are educating at the moment. :) Thanks.
Ok, if it the next patch I produce(aurora) ends up not differing from the default patch just landed, I will indicate to land the default patch on aurora.
(In reply to Henrik Skupin (:whimboo) from comment #96) > (In reply to Jonathan French (:jfrench) from comment #95) > Please only attach a new patch if it differs from default. Same applies for > the beta branch. If patches are identical we want to transplant them. Go ahead and land Default branch patch "follow-up for white-spaces (default)" on the Aurora branch. I have confirmed a new Aurora patch produced, does not differ from it. Same six files affected. Tested with Aurora 20.0a2 20130205042019. Tests pass where expected. I will look at Beta, after that.
The same "follow-up for white-spaces (default)" patch, doesn't look like it can be used for the Beta branch. It will need a subtly different patch, for issue outlined in Comment #44(excerpted below). The (correct)absence of this line in the Beta branch, means that the patch built in Default would not be applicable. I will upload a Beta specific patch shortly. (In reply to Jonathan French (:jfrench) from comment #44) > Issue One: > tests/functional/testSecurity/testGreyLarry.js > // Check the favicon has no label > this comment line existed in Beta branch up until this earlier change below > - which removed it. However - that line _exists, in the current head of the > Aurora branch > http://hg.mozilla.org/qa/mozmill-tests/diff/b9d820445e89/tests/functional/ > testSecurity/testGreyLarry.js > > One would have thought this line shouldn't exist in Aurora since the patch > was made after this return afaikt. My plan is to resolve this file for the > Beta branch with this line in question removed. ie. continuing its current > state, in Beta. > Good catch. And that's true. The patch on bug 803088 missed to remove that for > the default and aurora branch. There might also be other cases. If someone is > eager enough we can fix it but it's nothing critical.
Patch "follow-up for white-spaces (Beta)" for the Beta branch. Same six files affected and corrected, as with Default, and Aurora. However, subtle differences in one file, testGreyLarry, requiring a Beta-specific patch. Tested with Beta 19.0 20130130080006. Tests pass where expected.
Attachment #710896 - Flags: review?(hskupin)
Comment on attachment 710896 [details] [diff] [review] follow-up for white-spaces (Beta) Review of attachment 710896 [details] [diff] [review]: ----------------------------------------------------------------- Looks perfect. Thanks for the in-detail notes about this patch. Now lets get continued with the remaining patches for release and esr17. We are in a very good state now. http://hg.mozilla.org/qa/mozmill-tests/rev/8d3bcbd96504 (beta)
Attachment #710896 - Flags: review?(hskupin)
Attachment #710896 - Flags: review+
Attachment #710896 - Flags: checkin+
Right, continuing on with Release next.
Patch "initial-fix and white-spaces (Release)" for the Release branch. Eighteen files modified. Since neither of the other two initial Release patches were landed on Release branch, this patch represents the final desired state, the merging of both earlier patches, plus removal of unwanted end of file blank-lines(where applicable from the earlier patch). There are several tests in this return which contain eof blank-lines, and are in that state in the release branch. They have consciously been left untouched for continuity in the repo. Perhaps we can open a master bug for that issue, and address all tests that exhibit this problem, later. Both earlier Release patches have been superseded by this patch, and so have been deprecated. Tested with Release 18.0.2 20130201065344. Tests pass where expected.
Attachment #708608 - Attachment is obsolete: true
Attachment #708733 - Attachment is obsolete: true
Attachment #711490 - Flags: review?(hskupin)
Comment on attachment 711490 [details] [diff] [review] initial-fix and white-spaces (Release) Review of attachment 711490 [details] [diff] [review]: ----------------------------------------------------------------- > Bug 789916 - Initial fix for Release branch, replacing all instances of assertJSProperty() and assertDOMProperty() with new assert and expect methods - and incorporate all indent fixes introduced by the initial default patch. r=hskupin Uuiuii. Please keep that as short as possible. :) There isn't a need to specify any little detail in the commit. So in this case the message of the patch for default would work fine. Otherwise there is nothing I would have to add something. Looks fine. The white-space issues we can fix in a follow-up bug but it might worth to define the default check-in steps first to ensure we do not run into those problems again. I'm on vacation the next couple of days but maybe you can work with Dave Hunt on this in our automation mailing list? http://hg.mozilla.org/qa/mozmill-tests/rev/501d2f95ac75 (release)
Attachment #711490 - Flags: review?(hskupin)
Attachment #711490 - Flags: review+
Attachment #711490 - Flags: checkin+
So only esr17 is left to go. Then we can finally close this bug as fixed. yay!
status-firefox18: affected → fixed
Yup, continuing on with esr17 next. Sure, will submit to Dave for review when it's ready.
(In reply to Jonathan French (:jfrench) from comment #107) > Yup, continuing on with esr17 next. Sure, will submit to Dave for review > when it's ready. Look forward to it! :)
Go ahead and land Release branch patch "initial-fix and white-spaces (Release)" on the esr17 branch. I have confirmed a new esr17 patch produced, does not differ from it. Same eighteen files affected. I wanted to produce an esr17 patch just to satisfy myself the changes where what I expected them to be. Tested with 17.0.2esrpre 20130208034502. Tests pass where expected. Feel free to abbreviate the commit message as Henrik described, when landing the described Release patch, on Esr17. I had created this shortened version as guided, for my speculative esr17 patch which will remain unused: Bug 789916 - Replace controller.assertJSProperty() and controller.assertDOMProperty() in favor of expect.* and assert.* methods. r=dhunt Please also mark obsolete, this patch from the attachment list as it is superseded by that Release patch to be landed on Esr17 patchBug789916_Esr17_rev1_0 It doesn't appear I have any way of marking that attachment obsolete in Bugzilla, unless I am uploading a new patch. Maybe you guys have higher edit powers than I do, or I'm not seeing the Bugzilla edit functionality.
Attachment #709474 - Attachment is obsolete: true
Transplanted as: http://hg.mozilla.org/qa/mozmill-tests/rev/48c3f7a5a52f (mozilla-esr17) > It doesn't appear I have any way of marking that attachment obsolete in Bugzilla, unless I am uploading a new patch. Maybe you guys have higher edit powers than I do, or I'm not seeing the Bugzilla edit functionality. Maybe because you're new to Bugzilla you have limited privileges. You should be able to click 'details' next to the attachment, then 'edit details' and check 'obsolete'.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
status-firefox-esr17: affected → 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: