Currently: 26 warnings in total. Goal: 0.

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.

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.

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.

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.

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.

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!