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

Status: RESOLVED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

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.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: count the amount of warnings React generates

Comment 2

[Mark Banner (:standard8)]

Why only react? Why not any warnings?

Comment 3

[Mark Banner (:standard8)]

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

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: count the amount of warnings we generate

Comment 5

[Mike de Boer [:mikedeboer]]

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.

Comment 6

[Mark Banner (: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".

Comment 7

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=146aaa83a682

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8c5ada29852a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c5ada29852a

Comment 10

[Andrei Oprea [:andreio]]

> 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.

Comment 11

[Mike de Boer [:mikedeboer]]

(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.