Closed
Bug 632403
Opened 14 years ago
Closed 14 years ago
frame.event.fail() has to use the same failure structure as for exceptions
Categories
(Testing Graveyard :: Mozmill, defect)
Testing Graveyard
Mozmill
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: whimboo, Assigned: harth)
References
Details
(Whiteboard: [mozmill-1.5.2+][mozmill-2.0+])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
cmtalbert
:
review+
|
Details | Diff | Splinter Review |
At the moment the jumlib assert functions aren't compatible with the hard checks via exceptions. The former ones are using a different structure for the failure, which makes it kinda hard on our side to get the information from.
Right now it's mainly happening for our l10n test-run we have started recently. As it turned out, this test-run is pretty useful for our localizer community, and if possible we should get this fixed. I hope it will be an easy one.
Example:
http://mozmill-archive.brasstacks.mozilla.com/#/l10n/report/bb657a86662f7d3425d7730a3352bcae
http://mozmill-archive.brasstacks.mozilla.com/db/bb657a86662f7d3425d7730a3352bcae
{
"function": "jum.assert",
"comment": "accessKey: h found in string's: [id: (id is undefined), label: Mboa], [id: browserUseSystemColors, label: Fa syst\u025bm ahosu y\u025b adwuma]",
"value": false
},
{
"exception": {
"message": "Window has been found.",
"lineNumber": 449,
"stack": "waitFor([object Proxy],\"Window has been found.\")@resource://mozmill/modules/utils.js:449[..]",
"fileName": "resource://mozmill/modules/utils.js"
}
}
Clint, could we still shift this into 1.5.2? If not, I will have to workaround this problem. For 2.0 we definitely need a better solution. This week I will work on an assert method for our shared modules, which could become part of Mozmill 2.0 later on.
Updated•14 years ago
|
Whiteboard: [mozmill-1.5.2?] → [mozmill-1.5.2?][mozmill-2.0?]
Let's fix this simply for 1.5.2
For 2.0 let's be sure the new assertions model outputs properly.
Whiteboard: [mozmill-1.5.2?][mozmill-2.0?] → [mozmill-1.5.2+][mozmill-2.0+]
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → fayearthur+bugs
Assignee | ||
Comment 2•14 years ago
|
||
This patch will make jum failures throw Error()s like the controller and waitFor methods so the result will have the same structure (with stack, lineNumber).
Attachment #512636 -
Flags: review?(ctalbert)
Reporter | ||
Comment 3•14 years ago
|
||
Comment on attachment 512636 [details] [diff] [review]
throw Error()s instead of using frame.events.fail() in jum.js
No, please don't do that! Adding failures to the frame doesn't stop the test immediately, as what happens when throwing exceptions. We are in the unit test land here and it is extremely important to continue with the test.
Attachment #512636 -
Flags: feedback-
Reporter | ||
Comment 4•14 years ago
|
||
This would also break all kinds of test we have which make use of that unit testing approach.
Assignee | ||
Comment 5•14 years ago
|
||
Okay, this one doesn't throw an exception, just reports the error as if it were an exception. I actually don't know how I feel about this, if these aren't supposed to stop the tests, then I don't think they should be called "exceptions" in the results.
Attachment #512636 -
Attachment is obsolete: true
Attachment #512651 -
Flags: review?(ctalbert)
Attachment #512651 -
Flags: feedback?
Attachment #512636 -
Flags: review?(ctalbert)
Reporter | ||
Comment 6•14 years ago
|
||
(In reply to comment #5)
> Okay, this one doesn't throw an exception, just reports the error as if it were
> an exception. I actually don't know how I feel about this, if these aren't
> supposed to stop the tests, then I don't think they should be called
> "exceptions" in the results.
Otherwise we could also strip the exception out of the path and directly attach the failure object to the failure list. Not sure why we need the exception for failure we have thrown.
Assignee | ||
Comment 7•14 years ago
|
||
(In reply to comment #6)
> (In reply to comment #5)
> > Okay, this one doesn't throw an exception, just reports the error as if it were
> > an exception. I actually don't know how I feel about this, if these aren't
> > supposed to stop the tests, then I don't think they should be called
> > "exceptions" in the results.
>
> Otherwise we could also strip the exception out of the path and directly attach
> the failure object to the failure list. Not sure why we need the exception for
> failure we have thrown.
Hm, true. We could also call it "assertion" instead of "exception" if it's a unittest assertion, would that work for your results?
Reporter | ||
Comment 8•14 years ago
|
||
As long as the child elements will be the same as for exceptions, yes. That would even give me the change to visualize those assertions differently on the dashboard.
Assignee | ||
Comment 9•14 years ago
|
||
same deal, but using "assertion" in JSON instead of "exception"
Attachment #512651 -
Attachment is obsolete: true
Attachment #512677 -
Flags: review?(ctalbert)
Attachment #512651 -
Flags: review?(ctalbert)
Attachment #512651 -
Flags: feedback?
Comment 10•14 years ago
|
||
Comment on attachment 512677 [details] [diff] [review]
report jum failures as "assertion" with same structure as exceptions
I think this looks fine.
Pluggable events can't come soon enough. They should insulate us from future "wag the dog" requests like these.
For 2.0, can we get a unit test that essentially verifies that a failing JUM assert has the output we expect it to have? That way, we can ensure that when we refactor the assertion module from top to bottom we don't break this change.
r+ with test.
Attachment #512677 -
Flags: review?(ctalbert) → review+
Reporter | ||
Comment 11•14 years ago
|
||
Heather, something I missed yesterday. How do we report frame.event.pass? I believe we should this update too, even it is not included inside the report.
Assignee | ||
Comment 13•14 years ago
|
||
(In reply to comment #12)
> Heather, what do you think?
We report the same as we used to report JUM passes:
frame.events.pass({'function':'jum.assert', 'value':ifJSONable(booleanValue), 'comment':comment});
if we want to update it to be like the failures, we could do frame.events.pass({"assertion": message})
Doesn't matter much to me, I think we mainly pay attention to failures and this won't be around for long.
Assignee | ||
Comment 14•14 years ago
|
||
hotfix-1.5.2:
https://github.com/mozautomation/mozmill/commit/ce8539b928d86cf2949c08a0d3bdbeb581452024
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•