Catch/report reducer errors in sagaTester

kumar303 commented 6 years ago

Describe the problem and steps to reproduce it:

Create a saga
Make the saga dispatch an action like SET_THING
Make a reducer that processes SET_THING
Have the reducer throw an error in its switch/case block when it processes SET_THING
Create a test for the saga using the sagaTester pattern

What happened?

The test does not fail because of this exception but you will see its stack trace in your console.

Example:

    console.error node_modules/redux-saga/lib/internal/utils.js:240
      uncaught at handleReplyToReview Error: Cannot store reply to review ID 8876 because it does not exist
          at reviewsReducer (/Users/kumar/dev/addons-frontend/src/amo/reducers/reviews.js:202:17)
          at combination (/Users/kumar/dev/addons-frontend/node_modules/redux/lib/combineReducers.js:133:29)
          at finalReducer (/Users/kumar/dev/addons-frontend/node_modules/redux-saga-tester/dist/SagaTester.js:75:20)
          at dispatch (/Users/kumar/dev/addons-frontend/node_modules/redux/lib/createStore.js:178:22)
          at /Users/kumar/dev/addons-frontend/node_modules/redux-saga/lib/internal/middleware.js:72:22
          at /Users/kumar/dev/addons-frontend/node_modules/redux-saga-tester/dist/SagaTester.js:90:28
          at dispatch (/Users/kumar/dev/addons-frontend/node_modules/redux/lib/applyMiddleware.js:45:18)
          at /Users/kumar/dev/addons-frontend/node_modules/redux-saga/lib/internal/utils.js:265:12
          at /Users/kumar/dev/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:490:52
          at exec (/Users/kumar/dev/addons-frontend/node_modules/redux-saga/lib/internal/scheduler.js:25:5)
          at flush (/Users/kumar/dev/addons-frontend/node_modules/redux-saga/lib/internal/scheduler.js:66:5)
          at asap (/Users/kumar/dev/addons-frontend/node_modules/redux-saga/lib/internal/scheduler.js:39:5)
          at runPutEffect (/Users/kumar/dev/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:487:25)
          at runEffect (/Users/kumar/dev/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:436:184)
          at next (/Users/kumar/dev/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:317:9)
          at currCb (/Users/kumar/dev/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:389:7)
          at process._tickCallback (internal/process/next_tick.js:109:7)

What did you expect to happen?

The test should fail

Anything else we should know?

Maybe this is a saga-tester bug but I'm not sure. I haven't tried to reproduce it in a standalone codebase yet.

┆Issue is synchronized with this Jira Task

kumar303 commented 6 years ago

I suspected that the errorHandler was catching this but it is not. The error is not thrown in the saga at all.

kumar303 commented 6 years ago

I would really like to solve this. The first failure you see in this scenario is always the wrong failure.

kumar303 commented 6 years ago

Another example where you don't see a test failure at all 👻 https://github.com/mozilla/addons-frontend/pull/5501#pullrequestreview-135124011 and https://github.com/mozilla/addons-frontend/pull/5501#pullrequestreview-135482536

kumar303 commented 6 years ago

Bumping priority because these are causing silent test failures in some cases

willdurand commented 6 years ago

The issue comes from redux-saga, see: https://github.com/redux-saga/redux-saga/issues/1510. Apparently, upgrading to 1.0.0 (beta) fixes the issue...

We can either upgrade to 1.0.0 beta or use put.resolve() instead of put() everywhere. I guess upgrading would make sense 😋

willdurand commented 6 years ago

There are breaking changes: https://github.com/redux-saga/redux-saga/releases/tag/v1.0.0-beta.0. It is easy to make the changes on our codebase (and I believe things continue to work) but the SagaTester is not compatible yet. Even with fixing the SagaTester locally, we have to update our saga test cases...

willdurand commented 6 years ago

According to this: https://github.com/redux-saga/redux-saga/issues/632#issuecomment-277563541, we can use put.resolve() for now. We can fix our code by applying the following patch everywhere:

diff --git a/src/amo/sagas/home.js b/src/amo/sagas/home.js
index f79e69946..6e41d74f5 100644
--- a/src/amo/sagas/home.js
+++ b/src/amo/sagas/home.js
@@ -89,7 +89,7 @@ export function* fetchHomeAddons({
     return;
   }

-  yield put(
+  yield put.resolve(
     loadHomeAddons({
       collections,
       featuredExtensions: homeAddons.featuredExtensions,
diff --git a/tests/unit/amo/sagas/test_home.js b/tests/unit/amo/sagas/test_home.js
index 5afa1a8b7..f561a07fc 100644
--- a/tests/unit/amo/sagas/test_home.js
+++ b/tests/unit/amo/sagas/test_home.js
@@ -32,11 +32,13 @@ describe(__filename, () => {
   let mockCollectionsApi;
   let mockSearchApi;
   let sagaTester;
+  let sagaTesterError;

   beforeEach(() => {
     errorHandler = createStubErrorHandler();
     mockCollectionsApi = sinon.mock(collectionsApi);
     mockSearchApi = sinon.mock(searchApi);
+
     sagaTester = new SagaTester({
       initialState: dispatchClientMetadata().state,
       reducers: {
@@ -44,7 +46,15 @@ describe(__filename, () => {
         home: homeReducer,
       },
     });
-    sagaTester.start(homeSaga);
+
+    sagaTesterError = null;
+    sagaTester.start(homeSaga).done.catch((e) => {
+      sagaTesterError = e.stack;
+    });
+  });
+
+  afterEach(() => {
+    expect(sagaTesterError).toEqual(null);
   });

   describe('fetchHomeAddons', () => {

By applying this patch, the test suite does not pass anymore if a reducer throws an error and the error is displayed in the test logs. Below is the git diff of the reducer that throws an error, followed by the Jest output:

diff --git a/src/amo/reducers/home.js b/src/amo/reducers/home.js
index 438dad5eb..103b6db57 100644
--- a/src/amo/reducers/home.js
+++ b/src/amo/reducers/home.js
@@ -121,6 +121,8 @@ const reducer = (
       };

     case LOAD_HOME_ADDONS: {
+      throw new Error('oops');
+
       const {
         collections,
         featuredExtensions,

● /Users/williamdurand/projects/mozilla/addons-frontend/tests/unit/amo/sagas/test_home.js › fetchHomeAddons › loads a null for a collection that returns a 404

expect(received).toEqual(expected)

Expected value to equal:
  null
Received:
  "Error: oops
    at reducer (/Users/williamdurand/projects/mozilla/addons-frontend/src/amo/reducers/home.js:124:13)
    at result.(anonymous function) (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga-tester/dist/SagaTester.js:294:28)
    at combination (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux/lib/combineReducers.js:133:29)
    at finalReducer (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga-tester/dist/SagaTester.js:95:20)
    at dispatch (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux/lib/createStore.js:178:22)
    at /Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/middleware.js:72:22
    at /Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga-tester/dist/SagaTester.js:110:28
    at dispatch (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux/lib/applyMiddleware.js:45:18)
    at /Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/utils.js:265:12
    at /Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:500:52
    at exec (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/scheduler.js:25:5)
    at flush (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/scheduler.js:66:5)
    at asap (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/scheduler.js:39:5)
    at runPutEffect (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:497:25)
    at runEffect (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:446:184)
    at next (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:326:9)
    at currCb (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:399:7)
    at checkEffectEnd (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:623:9)
    at chCbAtKey (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:638:11)
    at currCb (/Users/williamdurand/projects/mozilla/addons-frontend/node_modules/redux-saga/lib/internal/proc.js:399:7)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)"

Difference:

  Comparing two different types of values. Expected null but received string.

  55 |
  56 |   afterEach(() => {
> 57 |     expect(sagaTesterError).toEqual(null);
     |                             ^
  58 |   });
  59 |
  60 |   describe('fetchHomeAddons', () => {

  at Object.<anonymous> (tests/unit/amo/sagas/test_home.js:57:29)

willdurand commented 6 years ago

/cc @kumar303

kumar303 commented 6 years ago

...but the SagaTester is not compatible yet.

We should wait until that's supported before upgrading.

We can fix our code by applying the following patch everywhere

That patch seems ok. I'd rather see a bit more abstraction around the beforeEach/afterEach saga handling, if possible.

willdurand commented 6 years ago

We should wait until that's supported before upgrading.

Not sure if they plan to upgrade though. Apparently, redux-saga 1.0 stable version should be released in a couple months.

That patch seems ok. I'd rather see a bit more abstraction around the beforeEach/afterEach saga handling, if possible.

Because everything is asynchronous, I am not sure I can abstract anything.

kumar303 commented 6 years ago

I lowered priority since this is blocked on a redux-saga release.

KevinMind commented 5 months ago

Old Jira Ticket: https://mozilla-hub.atlassian.net/browse/ADDFRNT-121

mozilla / addons