Countdown to zero warnings in desktop-local test suite

RESOLVED FIXED in Firefox 43

Status

P2
normal
Rank:
26
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mikedeboer, Assigned: frsela)

Tracking

unspecified
mozilla43
Points:
---
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox43 fixed)

Details

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

Attachments

(7 attachments, 5 obsolete attachments)

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
(Reporter)

Description

3 years ago
Currently: 190 warnings in total.
Goal: 0.
Flags: qe-verify-
Flags: firefox-backlog+

Updated

3 years ago
Rank: 26
Priority: -- → P2
Whiteboard: [test][tech-debt]
Assignee: nobody → frsela
Created attachment 8651644 [details] [diff] [review]
Reduce 17 warnings

I'll prepare more patch, not sure if you prefer stash all of them of land as independent patches ...
Attachment #8651644 - Flags: feedback?(mdeboer)
Created attachment 8651651 [details] [diff] [review]
Reduce 1 warnings
Attachment #8651651 - Flags: feedback?(mdeboer)
Attachment #8651644 - Attachment is patch: true
Attachment #8651644 - Attachment mime type: message/rfc822 → text/plain
Created attachment 8651656 [details] [diff] [review]
Reduce 1 warnings
Attachment #8651656 - Flags: feedback?(mdeboer)
(Reporter)

Comment 4

3 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

3 years ago
Attachment #8651644 - Flags: feedback+ → review+
(Reporter)

Comment 5

3 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

3 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+
(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
Created attachment 8652169 [details] [diff] [review]
Reduce 1 warning (nit fixed)

reverted incorrect change :)
Attachment #8651656 - Attachment is obsolete: true
Attachment #8652169 - Flags: review?(mdeboer)
(Reporter)

Updated

3 years ago
Attachment #8652169 - Flags: review?(mdeboer) → review+
Created attachment 8652740 [details] [diff] [review]
Reduce 1 warning

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)
(Reporter)

Comment 11

3 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

3 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)
Created attachment 8653272 [details] [diff] [review]
Reduce 1 warning (added e.preventDefault)
Attachment #8652740 - Attachment is obsolete: true
Attachment #8653272 - Flags: review?(mdeboer)
Created attachment 8653295 [details] [diff] [review]
Reduce 1 warning (added e.preventDefault)
Attachment #8653272 - Attachment is obsolete: true
Attachment #8653272 - Flags: review?(mdeboer)
Attachment #8653295 - Flags: review?(mdeboer)
Created attachment 8653384 [details] [diff] [review]
Reduce 3 warning
Attachment #8653384 - Flags: review?(mdeboer)
Created attachment 8653392 [details] [diff] [review]
Reduce 3 warnings
Attachment #8653392 - Flags: review?(mdeboer)
(Reporter)

Comment 17

3 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

3 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

3 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

3 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

3 years ago
Status: NEW → ASSIGNED
Flags: needinfo?(frsela)
(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)
(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...
Created attachment 8653875 [details] [diff] [review]
The last one \o/

No zarro boogs here ! :)
Attachment #8653875 - Flags: review?(mdeboer)
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-
(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
Created attachment 8653949 [details] [diff] [review]
Last patch fixed
Attachment #8653875 - Attachment is obsolete: true
Attachment #8653949 - Flags: review?(standard8)
Created attachment 8653958 [details] [diff] [review]
Last patch fixed V2
Attachment #8653949 - Attachment is obsolete: true
Attachment #8653949 - Flags: review?(standard8)
Attachment #8653958 - Flags: review?(standard8)
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+
https://hg.mozilla.org/mozilla-central/rev/d838f6278e74
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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.