Closed Bug 632403 Opened 13 years ago Closed 13 years ago

frame.event.fail() has to use the same failure structure as for exceptions

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
normal

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)

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.
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: nobody → fayearthur+bugs
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)
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-
This would also break all kinds of test we have which make use of that unit testing approach.
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)
(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.
(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?
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.
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 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+
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.
Heather, what do you think?
Status: NEW → ASSIGNED
(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.
hotfix-1.5.2:

https://github.com/mozautomation/mozmill/commit/ce8539b928d86cf2949c08a0d3bdbeb581452024
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Verified fixed with 1.5.2rc4.
Status: RESOLVED → VERIFIED
Blocks: 636764
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: