Closed Bug 1178393 Opened 4 years ago Closed 4 years ago

Countdown to zero warnings in standalone test suite

Categories

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

defect

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Iteration:
43.1 - Aug 24
Tracking Status
firefox43 --- fixed

People

(Reporter: mikedeboer, Assigned: crafuse)

References

Details

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

Attachments

(1 file, 2 obsolete files)

Currently: 26 warnings in total.
Goal: 0.
Flags: qe-verify-
Flags: firefox-backlog+
Summary: Countdown to zero warnings in shared test suite → Countdown to zero warnings in standalone test suite
Rank: 27
Priority: -- → P2
Whiteboard: [test][tech-debt]
Assignee: nobody → andrei.br92
Iteration: --- → 42.2 - Jul 27
Assignee: andrei.br92 → nobody
Iteration: 42.2 - Jul 27 → 42.3 - Aug 10
Assignee: nobody → chris.rafuse
Comment on attachment 8645026 [details] [diff] [review]
Countdown to zero warnings in standalone test suite

Fix warnings and errors in console output and expected test failures:
Added required isFirefox and unsupportedPlatform properties to standalone tests.
Added unloadWindow function with unit tests to stop listening to events.
Added ROOM_STATES.FULL and ROOM_STATES.ENDED types.
Attachment #8645026 - Flags: review?(andrei.br92)
Comment on attachment 8645026 [details] [diff] [review]
Countdown to zero warnings in standalone test suite

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

This looks great but I would like to add some more testing.

::: browser/components/loop/test/standalone/index.html
@@ +84,5 @@
>      });
>  
>      describe("Unexpected Warnings Check", function() {
>        it("should long only the warnings we expect", function() {
> +        chai.expect(caughtWarnings.length).to.eql(0);

Yay!

::: browser/components/loop/test/standalone/standaloneMetricsStore_test.js
@@ +205,5 @@
> +
> +      it("should stop listening to activeRoomStore", function() {
> +        sinon.stub(store, "stopListening");
> +        store.windowUnload();
> +        sinon.assert.calledOnce(store.stopListening);

I know I haven't probably mentioned this when we we're discussing the tech details but we probably want to test that `store.stopListening` is called with the correct argument (the active room store). So we need an extra assertion here.

::: browser/components/loop/test/standalone/webapp_test.js
@@ +113,5 @@
>        conversation.set("loopToken", "fakeToken");
>        ocView = mountTestComponent({
>          client: client,
>          conversation: conversation,
> +        isFirefox: true,

Seeing as this prop wasn't here before and we had no indication of that (besides the warnings) we probably want a test for this to make sure we are doing the right thing.

What we want:
* a new describe block for PromoteFirefoxView (if it doesn't exist)
  * test for isFirefox prop set to false
    - it should render and empty div, I think the way I would test this is see if `view.innerHTML === <div></div> or it might be an empty string, we'll see as we go`
  * test for isFirefox prop set to true
    - it should call mozL10n.get (and by this I mean a stub of it, create it if it does not exist) with `promote_firefox_hello_heading` and `get_firefox_button`
* tests for the views that include the `PromoteFirefoxView`
  * test that `CallUrlExpiredView` and `UnsupportedBrowsersView` load the promote firefox view when the isFirefox prop is set to false. The assertions should be similar to those described above with mozL10n.get.

I would also add this to the ui-showcase since I have no clue how it looks and since most of us test in Firefox we might want to keep an eye on this view in the showcase. 
To do this look in the ui-showcase.jsx for `2. Standalone webapp` and your steps are similar to how those views are included and added.
Attachment #8645026 - Flags: review?(andrei.br92) → review-
Notes from  Andrei Oprea [:andreio]:
RE: Promote firefox view
1. it should not return an empty div. that is due to the code being older and in a previous React version returning null from the render method was not possible. Change that to return null.
2. Here is an example on how I would test the null return 
http://jsbin.com/ludodofihi/1/edit?html,js,console,output
you can see that the console.log call there logs `1` you should do the same thing. 
Assert that querySelectorAll('noscript') is equal to 1.
Iteration: 42.3 - Aug 10 → 43.1 - Aug 24
Comment on attachment 8646012 [details] [diff] [review]
Countdown to zero warnings in standalone test suite

Added tests for promoteFirefoxView.
Added UI Showcase link to test index.html.
Attachment #8646012 - Flags: review?(andrei.br92)
Attachment #8645026 - Attachment is obsolete: true
Comment on attachment 8646012 [details] [diff] [review]
Countdown to zero warnings in standalone test suite

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

::: browser/components/loop/test/index.html
@@ +13,5 @@
>      <li><a href="shared/">Shared tests</a></li>
>      <li><a href="desktop-local/">Local tests</a></li>
>      <li><a href="standalone/">Standalone tests</a></li>
>      <li><a href="coverage/">Code Coverage</a></li>
> +    <li><a href="../ui/">UI Showcase</a></li>

Thanks!

::: browser/components/loop/test/standalone/standaloneMetricsStore_test.js
@@ +200,5 @@
> +      it("should call windowUnload when action is dispatched", function() {
> +        sandbox.stub(store, "windowUnload");
> +        dispatcher.dispatch(new sharedActions.WindowUnload());
> +        sinon.assert.calledOnce(store.windowUnload);
> +

No need for this empty line. Also please leave one empty line between test setup and all assertions (between line 202 and 203).

@@ +206,5 @@
> +
> +      it("should stop listening to activeRoomStore", function() {
> +        var stopListeningStub = sandbox.stub(store, "stopListening");
> +        store.windowUnload();
> +        sinon.assert.calledOnce(stopListeningStub);

A blank line above this as well.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +1527,5 @@
>        document.title = "Loop UI Components Showcase";
>  
>        // This simulates the mocha layout for errors which means we can run
>        // this alongside our other unit tests but use the same harness.
> +      var expectedWarningsCount = 16;

You didn't add the Promote Firefox view.
Attachment #8646012 - Flags: review?(andrei.br92) → review-
Attachment #8646012 - Attachment is obsolete: true
Attachment #8646472 - Flags: review?(andrei.br92)
Comment on attachment 8646472 [details] [diff] [review]
Countdown to zero warnings in standalone test suite

Added spacing and removed unnecessary empty lines.
Comment on attachment 8646472 [details] [diff] [review]
Countdown to zero warnings in standalone test suite

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

Great work. Thank you!
Attachment #8646472 - Flags: review?(andrei.br92) → review+
https://hg.mozilla.org/mozilla-central/rev/1175ddebd4f3
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.