1178390 - Countdown to zero warnings in desktop-local test suite

Status: RESOLVED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

Currently: 190 warnings in total.
Goal: 0.

Comment 1

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Reduce 17 warnings

I'll prepare more patch, not sure if you prefer stash all of them of land as independent patches ...

Comment 2

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Reduce 1 warnings

Comment 3

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Reduce 1 warnings

Comment 4

[Mike de Boer [:mikedeboer]]

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!

Comment 5

[Mike de Boer [:mikedeboer]]

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

Comment 6

[Mike de Boer [:mikedeboer]]

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.

Comment 7

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(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

Comment 8

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Reduce 1 warning (nit fixed)

reverted incorrect change :)

Comment 9

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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

Comment 10

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

I forgot to add the NI for comment #7

https://bugzilla.mozilla.org/show_bug.cgi?id=1178390#c7

Comment 11

[Mike de Boer [:mikedeboer]]

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?

Comment 12

[Mike de Boer [:mikedeboer]]

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

Comment 13

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Reduce 1 warning (added e.preventDefault)

Comment 14

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Reduce 1 warning (added e.preventDefault)

Comment 15

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Reduce 3 warning

Comment 16

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Reduce 3 warnings

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8653295 [details] [diff] [review]
Reduce 1 warning (added e.preventDefault)

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

LGTM

Comment 18

[Mike de Boer [:mikedeboer]]

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.

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8653392 [details] [diff] [review]
Reduce 3 warnings

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

Nice!

Comment 20

[Mike de Boer [:mikedeboer]]

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.

Comment 21

[Fernando R. Sela (no CC, needinfo please) [: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 :)

Comment 22

[Fernando R. Sela (no CC, needinfo please) [: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...

Comment 23

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: The last one \o/

No zarro boogs here ! :)

Comment 24

[Mark Banner (:standard8)]

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.

Comment 25

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(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

Comment 26

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Last patch fixed

Comment 27

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Last patch fixed V2

Comment 28

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

Comment 29

[Pulsebot]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d838f6278e74