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

VERIFIED FIXED

Status

VERIFIED FIXED
8 years ago
2 years ago

People

(Reporter: whimboo, Assigned: harth)

Tracking

Details

(Whiteboard: [mozmill-1.5.2+][mozmill-2.0+])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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?]

Comment 1

8 years ago
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

8 years ago
Assignee: nobody → fayearthur+bugs
(Assignee)

Comment 2

8 years ago
Created attachment 512636 [details] [diff] [review]
throw Error()s instead of using frame.events.fail() in jum.js

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

8 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

8 years ago
This would also break all kinds of test we have which make use of that unit testing approach.
(Assignee)

Comment 5

8 years ago
Created attachment 512651 [details] [diff] [review]
report JUM failures with same structure as exceptions

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

8 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

8 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

8 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

8 years ago
Created attachment 512677 [details] [diff] [review]
report jum failures as "assertion" with same structure as exceptions

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

8 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

8 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.
(Reporter)

Comment 12

8 years ago
Heather, what do you think?
Status: NEW → ASSIGNED
(Assignee)

Comment 13

8 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

8 years ago
hotfix-1.5.2:

https://github.com/mozautomation/mozmill/commit/ce8539b928d86cf2949c08a0d3bdbeb581452024
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 15

8 years ago
Verified fixed with 1.5.2rc4.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

8 years ago
Blocks: 636764
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.