Closed
Bug 1178390
Opened 9 years ago
Closed 9 years ago
Countdown to zero warnings in desktop-local test suite
Categories
(Hello (Loop) :: Client, defect, P2)
Hello (Loop)
Client
Tracking
(firefox43 fixed)
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: mikedeboer, Assigned: frsela)
References
Details
(Whiteboard: [test][tech-debt])
Attachments
(7 files, 5 obsolete files)
2.06 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
2.02 KB,
patch
|
mikedeboer
:
feedback+
|
Details | Diff | Splinter Review |
2.08 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
2.96 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
2.37 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
3.62 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
4.12 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
Currently: 190 warnings in total. Goal: 0.
Flags: qe-verify-
Flags: firefox-backlog+
Updated•9 years ago
|
Rank: 26
Priority: -- → P2
Whiteboard: [test][tech-debt]
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → frsela
Assignee | ||
Comment 1•9 years ago
|
||
I'll prepare more patch, not sure if you prefer stash all of them of land as independent patches ...
Attachment #8651644 -
Flags: feedback?(mdeboer)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8651651 -
Flags: feedback?(mdeboer)
Assignee | ||
Updated•9 years ago
|
Attachment #8651644 -
Attachment is patch: true
Attachment #8651644 -
Attachment mime type: message/rfc822 → text/plain
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8651656 -
Flags: feedback?(mdeboer)
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8651644 [details] [diff] [review] Reduce 17 warnings Review of attachment 8651644 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/test/desktop-local/conversationViews_test.js @@ +270,5 @@ > return TestUtils.renderIntoDocument( > React.createElement(loop.conversationViews.CallFailedView, { > dispatcher: dispatcher, > + contact: options.contact, > + outgoing: true Well, that's a slammer!
Attachment #8651644 -
Flags: feedback?(mdeboer) → feedback+
Reporter | ||
Updated•9 years ago
|
Attachment #8651644 -
Flags: feedback+ → review+
Reporter | ||
Comment 5•9 years ago
|
||
Comment on attachment 8651651 [details] [diff] [review] Reduce 1 warnings Review of attachment 8651651 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/test/desktop-local/conversationViews_test.js @@ +646,5 @@ > function() { > conversationStore.setStoreState({ > callState: CALL_STATES.TERMINATED, > + contact: contact, > + outgoing: true Looks the same as the previous patch...
Attachment #8651651 -
Flags: feedback?(mdeboer) → feedback+
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8651656 [details] [diff] [review] Reduce 1 warnings Review of attachment 8651656 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/test/desktop-local/conversationViews_test.js @@ +599,5 @@ > React.createElement(loop.conversationViews.CallControllerView, { > dispatcher: dispatcher, > mozLoop: fakeMozLoop, > + onCallTerminated: onCallTerminatedStub, > + Please revert this change.
Attachment #8651656 -
Flags: feedback?(mdeboer) → feedback+
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #5) > Comment on attachment 8651651 [details] [diff] [review] > Reduce 1 warnings > > Review of attachment 8651651 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/loop/test/desktop-local/conversationViews_test.js > @@ +646,5 @@ > > function() { > > conversationStore.setStoreState({ > > callState: CALL_STATES.TERMINATED, > > + contact: contact, > > + outgoing: true > > Looks the same as the previous patch... But in other place. Fixes another/different warning. We can stash both
Assignee | ||
Comment 8•9 years ago
|
||
reverted incorrect change :)
Attachment #8651656 -
Attachment is obsolete: true
Attachment #8652169 -
Flags: review?(mdeboer)
Reporter | ||
Updated•9 years ago
|
Attachment #8652169 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 9•9 years ago
|
||
This fixes this warning: Warning: Returning `false` from an event handler is deprecated and will be ignored in a future release. Instead, manually call e.stopPropagation() or e.preventDefault(), as appropriate.
Attachment #8652740 -
Flags: review?(mdeboer)
Assignee | ||
Comment 10•9 years ago
|
||
I forgot to add the NI for comment #7 https://bugzilla.mozilla.org/show_bug.cgi?id=1178390#c7
Flags: needinfo?(mdeboer)
Reporter | ||
Comment 11•9 years ago
|
||
Comment on attachment 8652740 [details] [diff] [review] Reduce 1 warning Review of attachment 8652740 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. Thanks! (getting closer ;) ) ::: browser/components/loop/content/js/conversationViews.jsx @@ +191,5 @@ > })); > > /* Prevent event propagation > * stop the click from reaching parent element */ > + e.stopPropagation(); Can you add `e.preventDefault()` for good measure here?
Attachment #8652740 -
Flags: review?(mdeboer) → review+
Reporter | ||
Comment 12•9 years ago
|
||
(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #7) > But in other place. Fixes another/different warning. > > We can stash both We agreed to merge all changes into a single patch before landing.
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8652740 -
Attachment is obsolete: true
Attachment #8653272 -
Flags: review?(mdeboer)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8653272 -
Attachment is obsolete: true
Attachment #8653272 -
Flags: review?(mdeboer)
Attachment #8653295 -
Flags: review?(mdeboer)
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8653384 -
Flags: review?(mdeboer)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8653392 -
Flags: review?(mdeboer)
Reporter | ||
Comment 17•9 years ago
|
||
Comment on attachment 8653295 [details] [diff] [review] Reduce 1 warning (added e.preventDefault) Review of attachment 8653295 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8653295 -
Flags: review?(mdeboer) → review+
Reporter | ||
Comment 18•9 years ago
|
||
Comment on attachment 8653384 [details] [diff] [review] Reduce 3 warning Review of attachment 8653384 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, not sure how this works, but doesn't seem to have side-effects.
Attachment #8653384 -
Flags: review?(mdeboer) → review+
Reporter | ||
Comment 19•9 years ago
|
||
Comment on attachment 8653392 [details] [diff] [review] Reduce 3 warnings Review of attachment 8653392 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
Attachment #8653392 -
Flags: review?(mdeboer) → review+
Reporter | ||
Comment 20•9 years ago
|
||
Fernando, can you fold these patches into one now so that I can land them for you? I'd really like to prevent this code from bitrotting.
Reporter | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Flags: needinfo?(frsela)
Assignee | ||
Comment 21•9 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #18) > Comment on attachment 8653384 [details] [diff] [review] > Reduce 3 warning > > Review of attachment 8653384 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hmm, not sure how this works, but doesn't seem to have side-effects. The warning was about no store was found for doing the required operations. So creating it solved the issue :)
Flags: needinfo?(frsela)
Assignee | ||
Comment 22•9 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #20) > Fernando, can you fold these patches into one now so that I can land them > for you? I'd really like to prevent this code from bitrotting. Fully agree, I'd only one pending warning but I didn't found the root cause yet. Do you want to land the fixed ones (folded, of course) or wait for this last one...
Assignee | ||
Comment 23•9 years ago
|
||
No zarro boogs here ! :)
Attachment #8653875 -
Flags: review?(mdeboer)
Comment 24•9 years ago
|
||
Comment on attachment 8653875 [details] [diff] [review] The last one \o/ Review of attachment 8653875 [details] [diff] [review]: ----------------------------------------------------------------- Stealing this as Mike is out today. ::: browser/components/loop/content/js/conversationAppStore.js @@ +31,5 @@ > > this._dispatcher.register(this, [ > "getWindowData", > + "showFeedbackForm", > + "setupWindowData" Unfortunately this causes an error, rather than a warning to be raised: TypeError: store[type] is not a function (which shows a limitation with what we're doing, we should be checking for zero errors as well, but lets save that for other bugs...). I would propose that we fix this the same way that we generally write our other test code now. Namely: - In the top-level beforeEach for "loop.store.ConversationAppStore", create a stub `sandbox.stub(dispatcher, "dispatch");` - Wherever we currently do dispatcher.dispatch, replace this with store.<actionName>, i.e. dispatcher.dispatch(new sharedActions.GetWindowData(fakeGetWindowData)); becomes store.getWindowData(new sharedActions.GetWindowData(fakeGetWindowData)); (note the change of case on the function name). This will mean we're only test the store function, and not the dispatcher, which is a better separation for the unit tests.
Attachment #8653875 -
Flags: review?(mdeboer) → review-
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #24) > Comment on attachment 8653875 [details] [diff] [review] > The last one \o/ > > Review of attachment 8653875 [details] [diff] [review]: > ----------------------------------------------------------------- > > (which shows a limitation with what we're doing, we should be checking for > zero errors as well, but lets save that for other bugs...). > Fully agree :D I didn't detect this bug, but changing the patch and move it to the test suite. Thank you
Assignee | ||
Comment 26•9 years ago
|
||
Attachment #8653875 -
Attachment is obsolete: true
Attachment #8653949 -
Flags: review?(standard8)
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8653949 -
Attachment is obsolete: true
Attachment #8653949 -
Flags: review?(standard8)
Attachment #8653958 -
Flags: review?(standard8)
Comment 28•9 years ago
|
||
Comment on attachment 8653958 [details] [diff] [review] Last patch fixed V2 Review of attachment 8653958 [details] [diff] [review]: ----------------------------------------------------------------- That's better, much nicer. Thank you.
Attachment #8653958 -
Flags: review?(standard8) → review+
Comment 30•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d838f6278e74
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•