Redux logic errors #1954

joshling1919 · 2019-11-16T02:47:35Z

Will need to add tests, but had some quick questions that I was hoping to get some feedback on.

For testing redux-logic, I was thinking about using https://github.com/jeffbski/redux-logic-test. Specifically, I would dispatch actions and then verify that the logic ran properly and resulted in the correct state. Not sure if this approach would assert on too many pieces outside of redux-logic. For example, it would depend on reducers being configured in a specific way. Would be interested in your thoughts!
I'm having a little bit of trouble wrapping my head around the way the original errors saga was set up, to where all of these would be running in parallel, and if there was already validation happening for a language, it would then cancel the validation call for that language. In Migrate from redux-saga to redux-logic #1934, you mentioned that redux-logic can't cancel in-flight asynchronous actions. Is this one of of the cases you were talking about, or was that something else?
Any thoughts on the organization of the redux-logic errors? I saw that @jwang1919 had split up to have one logic per file, but in terms of the errors saga, both validateCurrentProject and updateProjectSource called on the same validateSource function that will now not be its own logic, which is why I decided to organize it all into one file.

Thanks!

outoftime

Hey, great start here!

For testing redux-logic, I was thinking about using https://github.com/jeffbski/redux-logic-test. Specifically, I would dispatch actions and then verify that the logic ran properly and resulted in the correct state. Not sure if this approach would assert on too many pieces outside of redux-logic. For example, it would depend on reducers being configured in a specific way. Would be interested in your thoughts!

I’d prefer to avoid this, for the reason you mention: asserting on too many pieces outside of redux-logic. My philosophy on testing is that we should be very intentional about what parts of the code are and are not under test, and generally the answer to that question should respect organic code boundaries. There is not one right answer to where those boundaries belong—unit testing (boundary around a single class or module); functional testing (boundary around a particular functional concern, e.g. “the entire Redux store as a black box”); and integration testing (boundary around more or less the entire application, e.g. mounting the <App /> component and using react-testing-library to see what it does) are all useful testing tools.

While there is no right answer overall, and in fact the ideal testing strategy will almost certainly include test suites at more than one of these levels, Popcode’s current testing convention is to just write unit tests. This is for several reasons—partly historical (the React and Redux ecosystem strongly encouraged unit testing at the time Popcode’s testing conventions were established) but also because unit testing is the best way to test the behavior of our code under a wide range of cases. The further we get up the integration chain, the less natural it is to test edge-case behavior for each individual module; so, I’m of the opinion a good testing strategy is built on a foundation of comprehensive unit tests, with functional/integration tests giving us feedback about the integrated system, but with more of a focus on the happy path.

Given unlimited time, I would certainly want to build at least one integration testing suite for Popcode on top of the existing unit testing suite (after closing the current coverage gaps in unit testing), but since it’s a spare time project, I don’t have any expectation of that actually happening.

So: for now, I’d encourage you to write the tests for these logics in the same unit testing style as the existing logics, e.g. just test the behavior of the logic itself with any dependencies mocked out.

I'm having a little bit of trouble wrapping my head around the way the original errors saga was set up, to where all of these would be running in parallel, and if there was already validation happening for a language, it would then cancel the validation call for that language. In #1934, you mentioned that redux-logic can't cancel in-flight asynchronous actions. Is this one of of the cases you were talking about, or was that something else?

It’s definitely related, although cancelling an in-flight saga would generally mean aborting the asynchronous operation that the saga is waiting for; in this case all we need to do is ignore the result of validating a particular source if that source is modified again before the first validation completes. I hadn’t looked at this much before, but it looks like redux-logic does have an explicit affordance for cancellation after all. The nuance here is that we’ll only want to do this if there’s an in-flight validation for the same language; from skimming over the docs it’s not clear what the best way to do that is, but I’d love to hear about what you can find! And happy to dig in further if you get stuck.

Any thoughts on the organization of the redux-logic errors? I saw that @jwang1919 had split up to have one logic per file, but in terms of the errors saga, both validateCurrentProject and updateProjectSource called on the same validateSource function that will now not be its own logic, which is why I decided to organize it all into one file.

Addressed this one inline 🙂

outoftime · 2019-11-16T18:18:24Z

src/logic/errors.js

+  },
+});
+
+const updateProjectSource = createLogic({


I would actually probably call this validateProjectOnChange or something like that—generally I’d like to follow the principle of naming logics after what they actually do, rather than the action(s) they consume. (N.B. the sagas tend to follow the former naming convention, which is my fault!)

outoftime · 2019-11-16T18:20:10Z

src/logic/errors.js

+  const validate = validations[language];
+  const validationErrors = await validate(source, projectAttributes);
+  await dispatch(validatedSource(language, validationErrors));
+};


Rather than putting multiple logics in one module, let’s move this into its own module, probably inside a src/logic/helpers package that we can use in the general case when we need to share code between different logics. Then we can put each of the logics defined here in its own module.

outoftime · 2019-11-16T18:20:33Z

src/logic/errors.js

+import {validatedSource} from '../actions/errors';
+import retryingFailedImports from '../util/retryingFailedImports';
+
+export async function importValidations() {


Does this need to be exported?

joshling1919 · 2019-12-10T01:22:53Z

@outoftime so sorry, this took longer than expected but I've implemented most of your feedback! The only thing I still wasn't sure about was the cancellation/ignoring the validation of a source if there is already one in flight. I tried to experiment with and research on the topic a little more but I think I'm still a little confused on what the current problem is vs what the end goal should be when it comes to these cancellations. Is it that these validations are firing way more than necessary? What would be the best way to test that we've achieved the correct cancellation logic? Thanks for your help!

outoftime

Hey! Unfortunately running out of time to totally totally finish the review here but hopefully this is enough feedback to take another pass. As for the cancellation thing, I think using the latest option is probably sufficient for our purposes (you are using this in validateProjectOnSave but not validateCurrentProject; I would add it to the latter as well). What we’re looking to avoid is something like:

Project source updates to A
We fire validation on A
Project source updates to B
We fire validation on B
Validation on B completes
Validation on A completes
Thus we display outdated validation errors

Even with latest, this could still happen if the first validation were fired by validateCurrentProject and the second by validateProjectOnSave, or vice versa, since they won’t respect each others’ latest. If you want to be really ironclad, one approach would be to save a copy of the current project source before firing off the validation, and then when the validation returns, check that it hasn’t changed (because of the strong guarantees we have around immutability/referential stability in our Redux store, I think you should be able to do this with a very cheap referential equality check). I’m not 100% sure that that approach would cover all cases (e.g. what if there is a race between a validateCurrentProject and a validateProjectOnChange, where 2 of the 3 validations of the former are still legit when it completes) but might be worth exploring.

Overall, great work on this! Your code/tests are really solid and thoughtful 🙂

outoftime · 2019-12-18T12:13:06Z

src/logic/helpers/validateSource.js

+  );
+}
+
+export default async ({language, source, projectAttributes}, dispatch) => {


Extreme nit / not a blocker, but all else being equal I’d make this a regular named function rather than an arrow function (since it’s at the top level of the module / not being inlined in a larger expression).

outoftime · 2019-12-18T12:15:01Z

src/logic/validateCurrentProject.js

+    const validatePromises = [];
+    for (const language of Reflect.ownKeys(currentProject.sources)) {
+      const source = currentProject.sources[language];
+      validatePromises.push(
+        validateSource(
+          {language, source, projectAttributes: analyzer},
+          dispatch,
+        ),
+      );
+    }


Nit: a map would be a more idiomatic way to construct this array; I would probably use lodash’s map directly over currentProject.sources since it will iterate over object entries.

outoftime · 2019-12-18T12:17:49Z

src/logic/helpers/validateSource.js

+  const validations = await importValidations();
+  const validate = validations[language];
+  const validationErrors = await validate(source, projectAttributes);
+  await dispatch(validatedSource(language, validationErrors));


How come we’re awaiting the dispatch here? dispatch should be a void function; I wouldn’t expect it to return a promise…

outoftime · 2019-12-18T12:25:56Z

src/logic/__tests__/validateCurrentProject.test.js

+const mockCssSource = 'div {}';
+
+jest.mock('../../analyzers');
+jest.mock('../helpers/validateSource');


So this I think may be going a little further than we want with mocking—in particular the fact that validateCurrentProject delegates some of its logic to the validateSource helper is essentially an implementation detail of validateCurrentProject; if in the future we decided to pull all of that logic directly into validateCurrentProject, I don’t think we would consider that breaking from the standpoint of the unit tests.

So my proposal would be to just test the behavior of the validateCurrentProject logic here, including any behavior that happens to be implemented in validateSource; and remove the unit test for the validateSource helper.

outoftime · 2019-12-18T12:28:47Z

src/logic/__tests__/validateCurrentProject.test.js

+jest.mock('../../selectors', () => ({
+  getCurrentProject: jest.fn(() => ({
+    sources: {
+      html: mockHtmlSource,
+      css: mockCssSource,
+    },
+  })),
+}));


To be honest this also intuitively is probably not something I would have mocked if I were writing this test, although I think the argument is much less clear-cut than with the helper module commented on above. I could truly go either way on this, but wanted to flag it as something to think about, particularly in terms of “which decision is going to make this test more robust?”, with “robust” meaning “sensitive to breaking changes in the code under test, and insensitive to non-breaking changes in the code under test”.

outoftime · 2019-12-18T12:30:35Z

src/logic/__tests__/validateCurrentProject.test.js

+  const done = jest.fn();
+  await validateCurrentProject.process(
+    {
+      getState: () => dummyState,


I think eslint-plugin-lodash (introduced after you created this branch) is going to tell you to use constant here.

outoftime · 2019-12-18T12:33:11Z

src/logic/__tests__/validateCurrentProject.test.js

+  const analyzerPayload = getCurrentProject(dummyState);
+  expect(Analyzer).toHaveBeenCalledWith(analyzerPayload);
+  const analyzer = new Analyzer(analyzerPayload);
+  expect(validateSource.mock.calls).toEqual([


I would probably use expect.arrayContaining here since we’re not sensitive to the order of the calls.

outoftime · 2019-12-18T12:38:58Z

src/logic/__tests__/validateCurrentProject.test.js

+test('should validate current project', async () => {
+  const dispatch = jest.fn();
+  const done = jest.fn();
+  await validateCurrentProject.process(


So, this is a bit of a pain in the ass, but awaiting the call to process is actually testing implementation details of that method; from the standpoint of redux-logic, process does not have to be an async (or Promise-returning) function; it just needs to call done() when it’s done. I think the best way to test the relevant interface is something like:

await new Promise((resolve) => { validateCurrentProject.process( {getState: constant(dummyState)}, dispatch, resolve, ) });

FWIW I was looking through other logic tests for examples and am realizing I totally failed to catch a bunch of other cases of what you did here, so I’ll need to go back and fix those—sorry!!

Hey so! I dug into fixing this issue for the existing tests, and ended up just writing a test helper that actually runs logics through the redux-logic middleware, so the tests can focus on what the logics do rather than trying to mimic the implementation of redux-logic. Check out #2026 for all the sordid details!

joshling1919 · 2020-01-06T23:24:34Z

hi @outoftime! sorry just got back from winter break vacation. Thanks so much for the detailed and helpful feedback. Will take another pass and implement your feedback by end of week!

…edux-logic-errors

joshling1919 · 2020-01-14T01:44:02Z

@outoftime tried my best to implement your feedback. Please let me know if there was anything that I may have misunderstood!

Also, I was thinking more the case where there might be a race condition between validateCurrentProject and validateProjectOnChange. What are your thoughts on just merging the two into something that looks like:

export default createLogic({
  type: [
    'CHANGE_CURRENT_PROJECT',
    'GIST_IMPORTED',
    'SNAPSHOT_IMPORTED',
    'PROJECT_RESTORED_FROM_LAST_SESSION',
    'TOGGLE_LIBRARY',
    'UPDATE_PROJECT_SOURCE',
  ],
  latest: true,
  async process(
    {
      getState,
      action: {
        payload: {language, newValue},
      },
    },
    dispatch,
    done,
  ) {
    const state = getState();
    const currentProject = getCurrentProject(state);
    const analyzer = new Analyzer(currentProject);

    if (newValue) {
      await validateSource(
        {language, source: newValue, projectAttributes: analyzer},
        dispatch,
        getState,
    } else {
      const validatePromises = map(
        Reflect.ownKeys(currentProject.sources),
        language =>
          validateSource(
            {
              language,
              source: currentProject.sources[language],
              projectAttributes: analyzer,
            },
            dispatch,
            getState,
          ),
      );

      await Promise.all(validatePromises);
    }

    done();
  },
});

Not sure how you feel about using the conditional there to prevent unnecessary validations, but I think this might address the race condition.

…edux-logic-errors

outoftime · 2020-01-14T02:11:23Z

Right on! Will take a look as soon as I can. Looks like the build failed so that might be something to check out in the meantime 🙂

…edux-logic-errors

outoftime

Hey! This looks pretty much great; just a few notes on the way we generate data for tests.

In terms of your idea, it’s a good one! Feel free to implement it, I would just encourage you to:

Check the action name rather than the payload shape when deciding whether it’s single-source or full-project validation
Put each type of validation in its own function, so that the process() hook is just a multiplexer between the two

outoftime · 2020-01-17T01:25:41Z

src/logic/__tests__/validateCurrentProject.test.js

+  {
+    text: 'Closing tag missing',
+  },
+];


This seems like a pretty good candidate for a factory? Generally any time we need sample data to use in a test that conforms to a particular well-defined shape, a factory is the move (since it’s then reusable and rosie makes it super-easy to customize the generated objects for the needs of particular tests)

outoftime · 2020-01-17T01:27:17Z

src/logic/__tests__/validateCurrentProject.test.js

+  },
+];
+
+jest.mock('../../util/retryingFailedImports', () =>


Can we mock the validations module itself rather than the retryingFailedImports utility that we use to load it?

outoftime · 2020-01-17T01:30:29Z

src/logic/__tests__/validateCurrentProject.test.js

+        },
+      },
+    },
+  });


Generally when we’re testing against a particular starting state, I think it’s best to generate that state by running a series of actions through a reducer, rather than hand-rolling it—that way if we make changes to the state shape, updating the reducers will automatically update all the tests. Here’s a good example from another logic test.

outoftime · 2020-01-17T01:31:03Z

src/logic/__tests__/validateCurrentProject.test.js

+    snapshotImportedAction,
+    projectRestoredFromLastSessionAction,
+    toggleLibraryAction,
+  ].forEach(async action => {


EXTREME NITPICK: Why not for…of? 🙂

sorry, totally missed this comment

…edux-logic-errors

joshling1919 · 2020-01-25T20:49:19Z

@outoftime implemented your latest feedback!

I'm not sure if I implemented your vision of a multiplexor process function correctly though. When I initially tried to make it into two functions that the process function would delegate to, I found that there were too many arguments being passed around between functions and overall it felt like there was too much going on.

Let me know if the way I went with is okay. I'm not sure if I ended up making it less verbose at the expense of readability.

outoftime

Sooooo close! Only blocking issue is the async iterator function in the tests (which I think I might have missed on the last review, sorry!) Multiplexer thing is just polish so feel free to take or leave that feedback 🙂

outoftime · 2020-01-25T22:11:19Z

src/logic/__tests__/validateProject.test.js

+    snapshotImportedAction,
+    projectRestoredFromLastSessionAction,
+    toggleLibraryAction,
+  ].forEach(async action => {


Having an un-awaited async function as the iterator is going to prevent the assertions from being executed before the test completes—I would recommend iterating over the actions outside the test declaration, e.g.

for (const action of [ changeCurrentProjectAction, gistImportedAction, // ... ]) { test(`validates current project on ${action}`, async () => { // … } }

makes sense! will fix!

outoftime · 2020-01-25T22:24:42Z

src/logic/validateProject.js

+    const sources =
+      type === 'UPDATE_PROJECT_SOURCE'
+        ? {[language]: newValue}
+        : currentProject.sources;


So what I was thinking in terms of a multiplexer would be something like:

if (type === 'UPDATE_PROJECT_SOURCE') { await validateSource({language, source: newValue, projectAttributes: analyzer}, dispatch); } else { await validateSources(currentProject.sources, analyzer, dispatch) }

Not a big deal either way, but the above approach saves us an unnecessary hop through validateSources.

(Also, getting a bit nitpicky, I would probably try to be a bit more consistent about positional arguments vs. args object in the two function headers)

…popcode into redux-logic-errors

…edux-logic-errors

outoftime

Looks great!!

outoftime · 2020-01-31T12:22:42Z

src/logic/validateProject.js

+}
+
+async function validateSources({sources, projectAttributes}, dispatch) {
+  const validatePromises = map(Reflect.ownKeys(sources), language =>


Definitely not going to block on this, but a minor improvement here would be to map over sources itself, which would give you (source, language) as the arguments to the iterator.

This reverts commit 3314044.

joshling1919 added 4 commits November 12, 2019 17:03

Start migration updateProjectSource saga

4168920

Update updateProjectSource

c26af06

Use redux-logic for errors

fe8d719

Remove errors sagas unit tests

9bf3a0d

joshling1919 requested review from jwang1919 and outoftime and removed request for jwang1919 November 16, 2019 02:51

joshling1919 added the in progress label Nov 16, 2019

outoftime reviewed Nov 16, 2019

View reviewed changes

joshling1919 added 6 commits December 1, 2019 16:59

Reorganize errors logic

1227492

Start validateProjectOnChange tests

e40ff0f

Finish validateProjectOnChange specs

982f9d4

Write test for validateSource helper

e429d3b

Start validateCurrentProject test

7a6faef

Finish validateProjectOnChange tests

4e4e64c

joshling1919 removed the in progress label Dec 16, 2019

joshling1919 changed the title ~~[WIP] Redux logic errors~~ Redux logic errors Dec 16, 2019

joshling1919 requested a review from outoftime December 16, 2019 17:43

outoftime requested changes Dec 18, 2019

View reviewed changes

joshling1919 added 5 commits January 9, 2020 17:13

Merge branch 'master' of https://github.com/popcodeorg/popcode into r…

adfd45b

…edux-logic-errors

Refactor validateSource logic tests

9d7a55c

Lint validateCurrentProject and validateProjectOnChange

1080acb

Improve validateCurrentProject and validateProjectOnChange tests

33eb932

Ensure that only current validations are dispatched

80cf62f

joshling1919 requested a review from outoftime January 14, 2020 01:37

Merge branch 'master' of https://github.com/popcodeorg/popcode into r…

9a6a751

…edux-logic-errors

joshling1919 added 2 commits January 14, 2020 20:16

Merge branch 'master' of https://github.com/popcodeorg/popcode into r…

12b20a0

…edux-logic-errors

Lint errors logic

99c37ca

outoftime requested changes Jan 17, 2020

View reviewed changes

joshling1919 added 5 commits January 22, 2020 17:18

Merge branch 'master' of https://github.com/popcodeorg/popcode into r…

c2be8db

…edux-logic-errors

Start validateProject logic

5520295

Refactor errors logic to use validateProject

23c2613

Merge branch 'master' of https://github.com/popcodeorg/popcode into r…

c81b993

…edux-logic-errors

Run lint

c12991d

joshling1919 requested a review from outoftime January 25, 2020 20:49

Merge branch 'master' into redux-logic-errors

5a567a8

outoftime requested changes Jan 25, 2020

View reviewed changes

joshling1919 added 3 commits January 26, 2020 20:04

Refactor validateProject

3b666dd

Merge branch 'redux-logic-errors' of https://github.com/joshling1919/…

fbe3d0c

…popcode into redux-logic-errors

Merge branch 'master' of https://github.com/popcodeorg/popcode into r…

8826d1e

…edux-logic-errors

joshling1919 requested a review from outoftime January 27, 2020 04:23

Merge branch 'master' into redux-logic-errors

43505c8

outoftime approved these changes Jan 31, 2020

View reviewed changes

outoftime merged commit 3314044 into popcodeorg:master Jan 31, 2020

outoftime added a commit that referenced this pull request Jan 31, 2020

Revert "Redux logic errors (#1954)"

274c5bf

This reverts commit 3314044.

outoftime mentioned this pull request Jan 31, 2020

Revert "Redux logic errors" #2122

Merged

outoftime added a commit that referenced this pull request Jan 31, 2020

Revert "Redux logic errors (#1954)" (#2122)

df903da

This reverts commit 3314044.

outoftime mentioned this pull request Jan 31, 2020

Cannot read property 'toString' of undefined #2123

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redux logic errors #1954

Redux logic errors #1954

joshling1919 commented Nov 16, 2019 •

edited

Loading

outoftime left a comment

outoftime Nov 16, 2019

outoftime Nov 16, 2019

outoftime Nov 16, 2019

joshling1919 commented Dec 10, 2019

outoftime left a comment

outoftime Dec 18, 2019

outoftime Dec 18, 2019

outoftime Dec 18, 2019

outoftime Dec 18, 2019

outoftime Dec 18, 2019

outoftime Dec 18, 2019

outoftime Dec 18, 2019

outoftime Dec 18, 2019

outoftime Dec 24, 2019

joshling1919 commented Jan 6, 2020 •

edited

Loading

joshling1919 commented Jan 14, 2020 •

edited

Loading

outoftime commented Jan 14, 2020

outoftime left a comment

outoftime Jan 17, 2020

outoftime Jan 17, 2020

outoftime Jan 17, 2020

outoftime Jan 17, 2020

joshling1919 Jan 25, 2020

joshling1919 commented Jan 25, 2020

outoftime left a comment

outoftime Jan 25, 2020

joshling1919 Jan 25, 2020

outoftime Jan 25, 2020

outoftime left a comment

outoftime Jan 31, 2020

Redux logic errors #1954

Redux logic errors #1954

Conversation

joshling1919 commented Nov 16, 2019 • edited Loading

outoftime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshling1919 commented Dec 10, 2019

outoftime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshling1919 commented Jan 6, 2020 • edited Loading

joshling1919 commented Jan 14, 2020 • edited Loading

outoftime commented Jan 14, 2020

outoftime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshling1919 commented Jan 25, 2020

outoftime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outoftime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshling1919 commented Nov 16, 2019 •

edited

Loading

joshling1919 commented Jan 6, 2020 •

edited

Loading

joshling1919 commented Jan 14, 2020 •

edited

Loading