Closed Bug 828185 Opened 12 years ago Closed 12 years ago

Change as many as possible instances of "throw" to assert.* calls

Categories

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

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

Tracking

(firefox18 fixed, firefox19 fixed, firefox20 fixed, firefox21 fixed, firefox-esr17 fixed)

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

People

(Reporter: daniela.p98911, Assigned: daniela.p98911)

References

(
URL
)

Details

(Whiteboard: s=130128 u=enhancement c=assertions p=1)

Attachments

(2 files, 3 obsolete files)

patch v1.3
32.61 KB, patch
AndreeaMatei
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
patch v1.0 Beta, Release and ESR17
30.89 KB, patch
AndreeaMatei
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
All instances of "throw new Error(...)" should be changed with assert.* calls where possible.
Dave, that was my proposal which would make the code way cleaner. What do you think about? Here an example: if (condition) { throw new Error("My message"); } vs. assert.ok(condition, "My message");
Priority: -- → P3
Summary: Change the instances of "throw" with assert.* calls → Change as many as possible instances of "throw" to assert.* calls
See https://developer.mozilla.org/en/Mozmill_Tests how to get started.
URL: https://developer.mozilla.org/en/Mozm...
Whiteboard: [mentor=whimboo][lang=js][good first bug]
Whiteboard: [mentor=whimboo][lang=js][good first bug] → s=130128 u=enhancement c=assertions p=1
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
Attached patch patch v1.0 (obsolete) — — Splinter Review
patch v1.0 I have changed all instances of if(condition) throw... to assert.ok(!condition, message) - we are checking that the condition is true in assert and if it is false we throw exception. For instance: if (aRoot === undefined || aRoot === null) throw new Error("The root element has to be specified."); Changed to: assert.ok(aRoot !== undefined && aRoot !== null, "The root element has to be specified."); - we are evaluating if aRoot is not undefined/null and if it is undefined or null we are throwing the error
Attachment #708114 - Flags: review?(hskupin)
Attachment #708114 - Flags: review?(dave.hunt)
Attachment #708114 - Flags: review?(andreea.matei)
Comment on attachment 708114 [details] [diff] [review] patch v1.0 Review of attachment 708114 [details] [diff] [review]: ----------------------------------------------------------------- Good start Daniela! Overall I will also check for the cases where we declare the variable and then use assert.* for it, to make it in the same block. No need for an extra line. ::: lib/addons.js @@ +78,5 @@ > get discoveryPane() { > // Make sure the "Get Add-ons" pane is selected > var getAddons = this.getCategoryById({id: "discover"}); > > + assert.ok(this.selectedCategory.getNode() === getAddons.getNode(), We could use assert.equal here, that will also throw the error message if those are not equal. Please add a single quote after Add-ons in the message so we have " the 'Get Add-ons' pane " @@ +633,5 @@ > subtype: spec.attribute, > value: spec.value > }); > > + assert.ok(categories.length !== 0, arguments.callee.name + ": Categories could not be found."); We should use assert.notEqual() here. @@ +762,5 @@ > this.controller.window.document.removeEventListener("ViewChanged", > onViewChanged, false); > } > > + assert.ok(this.selectedCategory.getNode() === category.getNode(), We should use assert.equal() and check directly for the ids, this way we could avoid the got-expected part as well. ::: lib/prefs.js @@ +339,5 @@ > * (Optional) A callback handler to launch the preference dialog > */ > function openPreferencesDialog(controller, callback, launcher) { > + assert.ok(controller, "No controller given for Preferences Dialog"); > + assert.ok(typeof callback === "function", "No callback given for Preferences Dialog"); We can use assert.equal() here as well. ::: lib/software-update.js @@ +188,3 @@ > > var patchCount = this.activeUpdate.patchCount; > + assert.ok(patchCount === 1 || patchCount === 2, "An update must have one or two patches included."); Please move the message to the next line so it won't exceed the characters limit. ::: lib/ui/private-browsing.js @@ +89,5 @@ > var cmdKey = utils.getEntity(this.getDtds(), "privateBrowsingCmd.commandkey"); > aController.keypress(null, cmdKey, {accelKey: true, shiftKey: true}); > break; > case "callback": > + assert.ok(typeof aCallback === "function", "No callback given for opening the private browsing window"); Assert.equal() here too.
Attachment #708114 - Flags: review?(hskupin)
Attachment #708114 - Flags: review?(dave.hunt)
Attachment #708114 - Flags: review?(andreea.matei)
Attachment #708114 - Flags: review-
Attached patch patch v1.1 (obsolete) — — Splinter Review
Changes done per review
Attachment #708114 - Attachment is obsolete: true
Attachment #708569 - Flags: review?(hskupin)
Attachment #708569 - Flags: review?(dave.hunt)
Attachment #708569 - Flags: review?(andreea.matei)
Comment on attachment 708569 [details] [diff] [review] patch v1.1 Review of attachment 708569 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for missing on the previous review, but for the assert.ok calls at least, the message should be the positive/expected behaviour. Because our output will be "Expected behaviour - got false". Throw was displaying the exception. For example: assert.ok(name, arguments.callee.name + ": name not supplied."); should have "Name has been supplied", so in case of error we will get that expected behaviour got false. Same is required here: https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Error_Messages
Attachment #708569 - Flags: review?(hskupin)
Attachment #708569 - Flags: review?(dave.hunt)
Attachment #708569 - Flags: review?(andreea.matei)
Attachment #708569 - Flags: review-
Comment on attachment 709024 [details] [diff] [review] patch v1.2 Review of attachment 709024 [details] [diff] [review]: ----------------------------------------------------------------- Looks good now, but I would leave r? for Henrik and Dave, regarding the use of assert.equal().
Attachment #709024 - Flags: review?(andreea.matei) → review+
Comment on attachment 709024 [details] [diff] [review] patch v1.2 Review of attachment 709024 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/addons.js @@ +747,5 @@ > this.controller.window.document.removeEventListener("ViewChanged", > onViewChanged, false); > } > > + assert.equal(this.getCategoryId({category: this.selectedCategory}), categoryId, This check is wrong. You have to take the values from the if statement and not from the message. @@ +825,5 @@ > */ > set selectedSearchFilter(aValue) { > var filter = this.getSearchFilter({attribute: "value", value: aValue}); > > + assert.ok(SEARCH_FILTER.indexOf(aValue) !== -1, Clearly an assert.notEqual() call. ::: lib/dom-utils.js @@ +510,5 @@ > * @param {object} aRoot > * Element to use as root for node collection > */ > set root(aRoot) { > + assert.ok(aRoot !== undefined && aRoot !== null, "The root element has been specified."); this is just assert.ok(aRoot, ...) ::: lib/software-update.js @@ +183,5 @@ > get isCompleteUpdate() { > // Throw when isCompleteUpdate is called without an update. This should > // never happen except if the test is incorrectly written. > + assert.ok(this.activeUpdate, arguments.callee.name + ": isCompleteUpdate called " + > + "with an update"); Please do not modify the message. ::: lib/utils.js @@ +271,5 @@ > var elem = '<elem id="elementID">&' + entityId + ';</elem>'; > var doc = parser.parseFromString(header + elem, 'text/xml'); > var elemNode = doc.querySelector('elem[id="elementID"]'); > > + assert.notEqual(elemNode, null, this is an assert.ok(!elemNode, ...)
Attachment #709024 - Flags: review?(hskupin)
Attachment #709024 - Flags: review?(dave.hunt)
Attachment #709024 - Flags: review-
Comment on attachment 710158 [details] [diff] [review] patch v1.3 Review of attachment 710158 [details] [diff] [review]: ----------------------------------------------------------------- Great, think we're done now, for default.
Attachment #710158 - Flags: review?(andreea.matei) → review+
Comment on attachment 710158 [details] [diff] [review] patch v1.3 Review of attachment 710158 [details] [diff] [review]: ----------------------------------------------------------------- http://hg.mozilla.org/qa/mozmill-tests/rev/7e8af6459749 (default)
Attachment #710158 - Flags: checkin+
The patch for default applies clearly on Aurora and no additional instances of throw need to be changed. Reports for Aurora are: Addons: http://mozmill-crowd.blargon7.com/#/addons/report/1641bc2f6438595b86015bd9010db28f Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd9010da33b http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd9010d8cc9 Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/1641bc2f6438595b86015bd9010dd86e Update: http://mozmill-crowd.blargon7.com/#/update/report/1641bc2f6438595b86015bd9010e17ad http://mozmill-crowd.blargon7.com/#/update/report/1641bc2f6438595b86015bd9010e1ed0 The patch for Beta applies clearly on Release and ESR17. Reports for Beta are: Addons: http://mozmill-crowd.blargon7.com/#/addons/report/1641bc2f6438595b86015bd9010e2200 Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd9010ddcb9 http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd9010db2c8 Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/1641bc2f6438595b86015bd9010e090e Update: http://mozmill-crowd.blargon7.com/#/update/report/1641bc2f6438595b86015bd9010e3df0 http://mozmill-crowd.blargon7.com/#/update/report/1641bc2f6438595b86015bd9010e433e Reports for Release are: Addons: http://mozmill-crowd.blargon7.com/#/addons/report/1641bc2f6438595b86015bd9010ed1a9 Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd9010e997f http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd9010e79b5 Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/1641bc2f6438595b86015bd9010eaa5a Update: http://mozmill-crowd.blargon7.com/#/update/report/1641bc2f6438595b86015bd9010e5f7a http://mozmill-crowd.blargon7.com/#/update/report/1641bc2f6438595b86015bd9010e6170 Reports for ESR17 are: Addons: http://mozmill-crowd.blargon7.com/#/addons/report/1641bc2f6438595b86015bd90110941d Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd901105ce9 http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd901104333 Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/1641bc2f6438595b86015bd9011079f2 Update: http://mozmill-crowd.blargon7.com/#/update/report/1641bc2f6438595b86015bd9010ff515 http://mozmill-crowd.blargon7.com/#/update/report/1641bc2f6438595b86015bd9010fffe1
Attachment #710611 - Flags: review?(andreea.matei)
Comment on attachment 710611 [details] [diff] [review] patch v1.0 Beta, Release and ESR17 Review of attachment 710611 [details] [diff] [review]: ----------------------------------------------------------------- Applies cleanly to those branches, same changes, just none on the PB lib since there is none. Thanks Daniela!
Attachment #710611 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
status-firefox18: affected → fixed
status-firefox19: affected → fixed
status-firefox20: affected → fixed
status-firefox-esr17: affected → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
The patch for Beta got landed on Aurora. The patch for default needs to be landed on Aurora.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry that was my fault. Thanks for checking! http://hg.mozilla.org/qa/mozmill-tests/rev/e38882bd9467 (backout) http://hg.mozilla.org/qa/mozmill-tests/rev/eb6a9f4c7221 (transplant from default)
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
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: