refactor onLoadedProject() creator to handle success==false consistently; add tests of project state action creators #5362

benjiwheeler · 2020-01-06T20:23:14Z

Resolves

Resolves #5361

Proposed Changes

Adds unit tests for many project state reducer action creator functions.

Also refactors the project state reducer's onLoadedProject() function to be clearer, and to specifically handle failures to load the default project by switching to an error state.

(This relates to the project state reducer's handling of success and failure in loading the default project. Since we get that project data locally, and not from the server, loading it into the VM should always succeed. If it fails, we should switch to an error state.)

This change doesn't make a change to actual behavior, only to the project state reducer's API for clarity and consistency.

kchadha · 2020-01-23T18:48:46Z

src/reducers/project-state.js

@@ -435,10 +435,21 @@ const onLoadedProject = (loadingState, canSave, success) => {
        default:
            return;
        }
+    } else {


This code seems to be getting more confusing because of the combination of if and switch. I realize that's the way the code was already written, but for example, I think this else is not needed since there's a default: return above. However, that fact is not immediately obvious/easy to reason about.

Do you have any thoughts on how we could simplify this?

Thanks for pointing this out. I tried using one switch statement instead, and I think it's better:

switch (loadingState) { case LoadingState.LOADING_VM_WITH_ID: if (success) { return {type: DONE_LOADING_VM_WITH_ID}; } // failed to load project; just keep showing current project return {type: RETURN_TO_SHOWING}; case LoadingState.LOADING_VM_FILE_UPLOAD: if (success) { if (canSave) { return {type: DONE_LOADING_VM_TO_SAVE}; } return {type: DONE_LOADING_VM_WITHOUT_ID}; } // failed to load project; just keep showing current project return {type: RETURN_TO_SHOWING}; case LoadingState.LOADING_VM_NEW_DEFAULT: if (success) { return {type: DONE_LOADING_VM_WITHOUT_ID}; } // failed to load default project; show error return {type: START_ERROR}; default: return; }

This feels much better/easier to reason about!

kchadha · 2020-01-23T18:50:33Z

src/reducers/project-state.js

+        case LoadingState.LOADING_VM_WITH_ID:
+        case LoadingState.LOADING_VM_FILE_UPLOAD:
+            return {
+                type: RETURN_TO_SHOWING


What is the difference between RETURN_TO_SHOWING and START_ERROR? Since I am not familiar w/the project state machine, I think it would be helpful to have comments describing the state changes that are happening (this does not necessarily need to be added to this PR but would be useful to have in the future).

Thanks -- I'll put in a bit of commenting here, and I have done some experiments on how to add documentation to this file that I'd like to show you

kchadha · 2020-01-23T19:07:39Z

test/unit/reducers/project-state-reducer.test.js

+    expect(resultState.loadingState).toBe(LoadingState.SHOWING_WITHOUT_ID);
+});
+
+test('onLoadedProject (LOADING_VM_FILE_UPLOAD, false, false), with project id, shows with id', () => {


Not sure I understand what's going on in this test...

the test names are not exactly very clear! Let me try revising them.

Is the revised name more helpful?

benjiwheeler · 2020-01-27T21:24:54Z

Please take a look -- I revised a few things, and I think made it a bit clearer!

kchadha · 2020-02-04T21:42:05Z

src/reducers/project-state.js

+            return {type: DONE_LOADING_VM_WITHOUT_ID};
+        }
+        // failed to load default project; show error
+        return {type: START_ERROR};


Just to be clear, this is the piece of logic that is new to this PR, right?
The rest of this is chunk of code is a refactor?

kchadha · 2020-02-04T21:44:05Z

test/unit/reducers/project-state-reducer.test.js

+    const initialState = {
+        loadingState: LoadingState.LOADING_VM_WITH_ID
+    };
+    const action = onLoadedProject(initialState.loadingState, true, true);


Is this one supposed to be false, true? It looks like this test is exactly the same as the one above...

benjiwheeler · 2020-02-10T19:40:49Z

test/unit/reducers/project-state-reducer.test.js

+    expect(resultState.loadingState).toBe(LoadingState.SHOWING_WITH_ID);
+});
+
+test('onLoadedProject(LOADING_VM_WITH_ID, false, false), with no project id, ' +


What should happen here? State is LOADING_VM_WITH_ID, but projectId is null -- is that even possible?

Would be good to add a comment here explaining that this is for the case where we started viewing a project that doesn't have an ID (e.g. in standalone mode where we go from viewing a project w/o an ID and then add an id using the #.... method in the URL)

kchadha · 2020-02-10T19:47:27Z

test/unit/reducers/project-state-reducer.test.js

+test('onLoadedProject(LOADING_VM_WITH_ID, false, false), with project id, ' +
+    'results in state SHOWING_WITH_ID', () => {


Could you add a comment here that explains why it results in state "SHOWING_WITH_ID"? That we're showing the original project w/ID (not the one that failed to load)

benjiwheeler · 2020-02-10T20:06:43Z

test/unit/reducers/project-state-reducer.test.js

+        loadingState: LoadingState.LOADING_VM_FILE_UPLOAD,
+        projectId: '12345'
+    };
+    const action = onLoadedProject(initialState.loadingState, false, false);
    const resultState = projectStateReducer(initialState, action);
    expect(resultState.loadingState).toBe(LoadingState.SHOWING_WITH_ID);


We should be also checking that the projectid is what we expect -- in each expect line

benjiwheeler · 2020-02-26T22:19:36Z

@kchadha Made the changes we discussed!

kchadha · 2020-03-02T21:49:15Z

test/unit/reducers/project-state-reducer.test.js

+test('onLoadedProject(LOADING_VM_WITH_ID, false, false), with no project id, ' +
+    'results in state SHOWING_WITHOUT_ID', () => {
+    const initialState = {
+        projectId: '0',


Hmmm did the nature of this test change here by adding the projectId field? E.g. is it the same test as the one above or does the '0' here have special meaning that exercises a different code path?

That's a good question. '0' is the specific projectId value of the default project. It's defined at the top of the project-state reducer file.

So this test's expecting to reach the state SHOWING_WITHOUT_ID, whereas the test above is expecting to reach the state SHOWING_WITH_ID.

kchadha

Had one last question about one of the tests, but otherwise LGTM

benjiwheeler · 2020-03-06T04:11:37Z

@kchadha I'm realizing I named this PR poorly at the start; this was intended as a change in project state reducer to support further changes in vm-manager-hoc.

So this change only changes the project state reducer's onLoadedProject() action creator to be more consistent about how it will handle the cases when the success parameter is false. It doesn't actually change functionality yet -- it just sets us up to do that next.

The change it was saying it made -- switching to an error state if loading the default project fails -- is actually satisfied by gui's current functionality! It's other cases that don't work as intended: if loading fails for a different fetched project, currently gui will enter an error state without even calling onLoadedProject().

After this change, we would/will be able to change vm-manager-hoc to call onLoadedProject() with the success parameter false, so that it can go into an error state for the default project, but avoid going into one for other projects.

benjiwheeler · 2020-03-06T04:12:03Z

Also, I replied to your outstanding question on that test!

benjiwheeler · 2020-03-06T04:30:05Z

Here's a patch that you can apply locally, which will make vm loading fail 50% of the time, and call onLoadedProject() with the resulting success or failure:

diff --git a/src/containers/sb-file-uploader.jsx b/src/containers/sb-file-uploader.jsx
index 2019f719e..14e2a2ba9 100644
--- a/src/containers/sb-file-uploader.jsx
+++ b/src/containers/sb-file-uploader.jsx
@@ -127,6 +127,8 @@ class SBFileUploader extends React.Component {
             const filename = this.fileToUpload && this.fileToUpload.name;
             this.props.vm.loadProject(this.reader.result)
                 .then(() => {
+                    const success = (Math.random() > .5);
+                    if (!success) throw "random error";
                     this.props.onLoadingFinished(this.props.loadingState, true);
                     // Reset the file input after project is loaded
                     // This is necessary in case the user wants to reload a project
diff --git a/src/lib/vm-manager-hoc.jsx b/src/lib/vm-manager-hoc.jsx
index 7947dfd64..42170df2b 100644
--- a/src/lib/vm-manager-hoc.jsx
+++ b/src/lib/vm-manager-hoc.jsx
@@ -54,7 +54,9 @@ const vmManagerHOC = function (WrappedComponent) {
         loadProject () {
             return this.props.vm.loadProject(this.props.projectData)
                 .then(() => {
-                    this.props.onLoadedProject(this.props.loadingState, this.props.canSave);
+                    const success = (Math.random() > .5);
+                    if (!success) throw "random error";
+                    this.props.onLoadedProject(this.props.loadingState, this.props.canSave, true);
                     // Wrap in a setTimeout because skin loading in
                     // the renderer can be async.
                     setTimeout(() => this.props.onSetProjectUnchanged());
@@ -71,7 +73,7 @@ const vmManagerHOC = function (WrappedComponent) {
                     }
                 })
                 .catch(e => {
-                    this.props.onError(e);
+                    this.props.onLoadedProject(this.props.loadingState, this.props.canSave, false);
                 });
         }
         render () {
@@ -137,8 +139,8 @@ const vmManagerHOC = function (WrappedComponent) {

     const mapDispatchToProps = dispatch => ({
         onError: error => dispatch(projectError(error)),
-        onLoadedProject: (loadingState, canSave) =>
-            dispatch(onLoadedProject(loadingState, canSave, true)),
+        onLoadedProject: (loadingState, canSave, success) =>
+            dispatch(onLoadedProject(loadingState, canSave, success)),
         onSetProjectUnchanged: () => dispatch(setProjectUnchanged())
     });

When I run gui locally with this, what I see is

errors for loading the default project result in an error screen
errors for loading any other project result in still showing the editor normally

...which is what's desired.

kchadha

LGTM! Thanks for all the conversation on this.

benjiwheeler added 3 commits January 6, 2020 15:10

project state loading should error if can’t load default

461877b

added tests for project loading states

50265fe

fixed typo

a4e2711

benjiwheeler added the pr - needs review label Jan 6, 2020

benjiwheeler added this to the January 2020 milestone Jan 6, 2020

benjiwheeler requested a review from kchadha January 6, 2020 20:23

benjiwheeler assigned kchadha Jan 6, 2020

benjiwheeler changed the title ~~Project state tests~~ if loading the default project is not successful, go to error state Jan 6, 2020

kchadha reviewed Jan 23, 2020

View reviewed changes

kchadha added pr - changes requested and removed pr - needs review labels Jan 23, 2020

kchadha assigned benjiwheeler and unassigned kchadha Jan 23, 2020

reorganized switch and conditionals to be clearer; clarified test names

3a8fe3f

benjiwheeler added pr - needs review and removed pr - changes requested labels Jan 27, 2020

benjiwheeler assigned kchadha and unassigned benjiwheeler Jan 27, 2020

benjiwheeler mentioned this pull request Feb 3, 2020

refactor SB* file uploader into top-level HOC #4809

Merged

benjiwheeler modified the milestones: January 2020, February 2020 Feb 4, 2020

kchadha reviewed Feb 4, 2020

View reviewed changes

benjiwheeler commented Feb 10, 2020

View reviewed changes

kchadha reviewed Feb 10, 2020

View reviewed changes

benjiwheeler commented Feb 10, 2020

View reviewed changes

test resulting projectId; add comments; fix params

2e11787

benjiwheeler force-pushed the project-state-tests branch from 41f6427 to 2e11787 Compare February 26, 2020 22:05

kchadha reviewed Mar 2, 2020

View reviewed changes

kchadha assigned benjiwheeler and unassigned kchadha Mar 4, 2020

benjiwheeler changed the title ~~if loading the default project is not successful, go to error state~~ add project state reducer tests of action creator functions; refactor onLoadedProject() creator Mar 6, 2020

benjiwheeler changed the title ~~add project state reducer tests of action creator functions; refactor onLoadedProject() creator~~ refactor onLoadedProject() creator to handle success==false consistently; add tests of project state action creators Mar 6, 2020

benjiwheeler assigned kchadha and unassigned benjiwheeler Mar 6, 2020

benjiwheeler modified the milestones: February 2020, March 2020 Mar 11, 2020

kchadha approved these changes Mar 18, 2020

View reviewed changes

kchadha added pr - good to merge and removed pr - needs review labels Mar 18, 2020

benjiwheeler merged commit d0eb28f into scratchfoundation:develop Mar 18, 2020

kchadha assigned benjiwheeler and unassigned kchadha Mar 18, 2020

benjiwheeler deleted the project-state-tests branch March 18, 2020 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor onLoadedProject() creator to handle success==false consistently; add tests of project state action creators #5362

refactor onLoadedProject() creator to handle success==false consistently; add tests of project state action creators #5362

benjiwheeler commented Jan 6, 2020 •

edited

Loading

kchadha Jan 23, 2020

benjiwheeler Jan 27, 2020

kchadha Feb 4, 2020

kchadha Jan 23, 2020

benjiwheeler Jan 27, 2020

kchadha Jan 23, 2020

benjiwheeler Jan 27, 2020

benjiwheeler Jan 27, 2020

benjiwheeler commented Jan 27, 2020

kchadha Feb 4, 2020

benjiwheeler Feb 26, 2020

kchadha Feb 4, 2020

benjiwheeler Feb 10, 2020

benjiwheeler Feb 26, 2020

benjiwheeler Feb 10, 2020

kchadha Feb 10, 2020 •

edited

Loading

benjiwheeler Feb 26, 2020

kchadha Feb 10, 2020

benjiwheeler Feb 26, 2020

benjiwheeler Feb 10, 2020

benjiwheeler Feb 26, 2020

benjiwheeler commented Feb 26, 2020

kchadha Mar 2, 2020

benjiwheeler Mar 6, 2020

kchadha left a comment

benjiwheeler commented Mar 6, 2020

benjiwheeler commented Mar 6, 2020

benjiwheeler commented Mar 6, 2020

kchadha left a comment

		test('onLoadedProject(LOADING_VM_WITH_ID, false, false), with project id, ' +
		'results in state SHOWING_WITH_ID', () => {

refactor onLoadedProject() creator to handle success==false consistently; add tests of project state action creators #5362

refactor onLoadedProject() creator to handle success==false consistently; add tests of project state action creators #5362

Conversation

benjiwheeler commented Jan 6, 2020 • edited Loading

Resolves

Proposed Changes

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

benjiwheeler commented Jan 27, 2020

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

kchadha Feb 10, 2020 • edited Loading

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

benjiwheeler commented Feb 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchadha left a comment

Choose a reason for hiding this comment

benjiwheeler commented Mar 6, 2020

benjiwheeler commented Mar 6, 2020

benjiwheeler commented Mar 6, 2020

kchadha left a comment

Choose a reason for hiding this comment

benjiwheeler commented Jan 6, 2020 •

edited

Loading

kchadha Feb 10, 2020 •

edited

Loading