Closed Bug 1178383 Opened 6 years ago Closed 6 years ago

Add tests to not not allow more warnings to be generated for each mocha test suite

Categories

(Hello (Loop) :: Client, defect, P2)

defect
Points:
2

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Iteration:
42.1 - Jul 13
Tracking Status
firefox42 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: [test][tech-debt])

Attachments

(1 file, 1 obsolete file)

To do this, we'll start collecting warnings as logged to the console by React.js and compare the number of warnings at the end of the run with the amount we expect.
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Why only react? Why not any warnings?
Also, I think you should be able to do something similar for the ui-showcase. See the setTimeout in the DOMContentLoaded listener at the bottom of ui-showcase.jsx
Attachment #8627275 - Attachment is obsolete: true
Attachment #8627275 - Flags: review?(dmose)
Attachment #8627607 - Flags: review?(dmose)
Comment on attachment 8627607 [details] [diff] [review]
Patch v2: count the amount of warnings we generate

Since I made bug 1171940 depend on this bug, I'd like to get this in before that.
Attachment #8627607 - Flags: review?(dmose) → review?(standard8)
Comment on attachment 8627607 [details] [diff] [review]
Patch v2: count the amount of warnings we generate

Review of attachment 8627607 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. You might just want to give it a quick check on Windows & Linux on try - just in case there's a different count for some reason. Up to you.

::: browser/components/loop/test/desktop-local/index.html
@@ +95,5 @@
>        });
>      });
>  
> +    describe("Unexpected Warnings Check", function() {
> +      it("should not log more warnings than we expect", function() {

Minor nit, might be clearer as "should long only the warnings we expect".
Attachment #8627607 - Flags: review?(standard8) → review+
Iteration: 42.3 - Aug 10 → 42.1 - Jul 13
Rank: 25
Priority: -- → P2
Whiteboard: [test][tech-debt]
https://hg.mozilla.org/mozilla-central/rev/8c5ada29852a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
> chai.expect(caughtWarnings.length).to.eql(128);

This shouldn't have been a strict equality check because but a `<=` imo. Reducing warnings means this test will fail. I addressed this in the NPS patch and set it to `<=` and also lowered the number after fixing some parts of the code.
(In reply to Andrei Oprea [:andreio] from comment #10)
> > chai.expect(caughtWarnings.length).to.eql(128);
> 
> This shouldn't have been a strict equality check because but a `<=` imo.
> Reducing warnings means this test will fail. I addressed this in the NPS
> patch and set it to `<=` and also lowered the number after fixing some parts
> of the code.

No! If we set this to be lesser or equal, we'll never get a grasp on what the current level of warnings is. If you provide patches that lower the amount of warnings, you can simply update the amount of warnings in the respective index.html file and we'll _know_ that we're actively lowering this amount closer to 0.
You need to log in before you can comment on or make changes to this bug.