ProjectStatus unit tests #4127

jelliotartz · 2017-10-30T20:20:34Z

Describe your changes

Adds tests for the ProjectStatus component in Admin.
Tests are divided into a group that checks the presence of the basic page layout elements, and a group that focuses on the Workflow Settings section (where I've recently implemented changes regarding toggling active status of default workflows)
- RFC: Not sure if testing page layout elements is necessary, or a good practice. Happy to remove if not needed/wanted.
RFC: So far I haven't been able to write tests that simulate a user clicking the WorkflowToggle checkbox, getting the WorkflowDefaultDialog modal to open, and testing its functionality.
- Are tests of the modal functionality (eg, user clicks the 'active' toggle for a default workflow, modal opens, user clicks 'ok', and default_workflow gets removed from project) outside the scope of testing the ProjectStatus component?
- Would appreciate input on a) are tests of modal functionality necessary/desired/good practice? b) if yes, any suggestions on where my syntax is off?
  - Is there anywhere in the PFE codebase where we use Enzyme to test modal dialogs being triggered in parent components?
  - I thought perhaps I needed to use mount instead of shallow render, but haven't had success with mount either.
  - Do I need to use a stub instead of a spy, and control/force the code down a specific path (ie, to click the toggle for a default workflow, then force 'ok'/'cancel')
- Is it possible that testing modals in Enzyme is difficult/buggy? some SO and github issues I've found while trying to get modal tests working:
Related PRs (my recent work related to ProjectStatus component and toggling default workflow status):
- Add check for default workflow #4118 (default workflow existential check)
- Workflow default dialog alert #4100 (default workflow dialog alert)
- WorkflowToggle ES6 conversion #4092 (WorkflowToggle ES6 conversion)

Review Checklist

Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
Does it work on mobile?
Can you rm -rf node_modules/ && npm install and app works as expected?
Did you deploy a staging branch?
If the component is in coffeescript, is it converted to ES6? Is it free of eslint errors? Is the conversion its own commit?
Are the tests passing locally and on Travis?

Optional

Have you replaced any ChangeListener or PromiseRenderer components with code that updates component state?
If changes are made to the classifier, does the dev classifier still work?
Have you resized and compressed any image you've added?
Have you added in flow type annotations?
Have you followed the Springer guidelines for commit messages?

srallen

Not sure if testing page layout elements is necessary, or a good practice. Happy to remove if not needed/wanted.

No, we do not need to re-test what React and JSX is doing for us for standard HTML elements. We trust that our dependencies are tested and we should not spend time re-testing core functionality of them. We do we want to test that our custom React components are rendered.

So far I haven't been able to write tests that simulate a user clicking the WorkflowToggle checkbox, getting the WorkflowDefaultDialog modal to open, and testing its functionality.

Testing the interaction between components are integration tests. Just focus on writing unit tests for this component. The dialog component already has its own unit tests written, too.

Let me know if you need further clarification on what should be covered.

jelliotartz · 2017-11-01T21:13:00Z

PR Update

Thanks for your input @srallen :) RFC: I was writing a test for rendering the WorkflowDefaultDialog component, and came upon an issue - the component is currently rendered once for every workflow, rather than just once, for the default workflow. (you can test this by inspecting the modal element on a project with multiple workflows).

This seems like an issue that should be resolved, and I've refactored the code in ProjectStatus to only render one WorkflowDefaultDialog component when the modal is triggered (on a separate branch, not currently included here). However, updating ProjectStatus to only render one WorkflowDefaultDialog seems like it's outside of the scope of this PR. But, the issue was discovered in the process of writing the test for the presence of the component, which is within the scope of the PR...

I pushed up changes that include a passing test for the WorkflowDefaultDialog component (it tests whether it renders one for each workflow), with a comment to refactor ProjectStatus so that we only render one component. Happy to either update ProjectStatus and the test within this PR, or to open up a separate PR to address the issue and fix the test there, just wanted to confer before continuing :)

srallen · 2017-11-02T18:14:06Z

Let's get the fix in with this. Just keep the tests and the bug fix as separate commits.

jelliotartz · 2017-11-02T20:38:38Z

app/pages/admin/project-status.jsx

      this.setState({
        dialogIsOpen: true
      });
    }

-    if ((defaultWorkflowId !== workflow.id) || (defaultWorkflowId === workflow.id && !workflow.active)) {
+    if ((this.state.defaultWorkflowId !== workflow.id) || (this.state.defaultWorkflowId === workflow.id && !workflow.active)) {


As I was updating this conditional, I was wondering if it's necessary - I tried changing it to just be an else attached to the conditional above it, and the UI functionality seems to be the same (tests all still passing as well).

Would appreciate input on whether it's necessary here to explicitly check for the opposite of the conditions described above.

jelliotartz · 2017-11-02T20:39:06Z

PR Update

Bug fix for modal added on a separate commit.
I've added unit tests for the custom components, which I put in their own describe block.
I created a 'workflow settings' describe block, where I unit test each of the custom components associated with workflow settings, as well as the #onChangeWorkflowLevel method.
I'm not sure if I need to write tests for other methods in this component, (e.g., project and workflow GET requests, componentDidMount, and the event handler methods). Please let me know if more tests need to be written to cover unit testing for this component :)

srallen

I'm not sure if I need to write tests for other methods in this component, (e.g., project and workflow GET requests, componentDidMount, and the event handler methods).

Event handlers should be unit tested with stubs, mocks, or spies.
componentDidMount depends on the component. Shallow rendering doesn't fire this lifecycle method. In this case cdm is calling a method that is requesting data from the API
Testing the interaction with the API are integration tests, so for this PR, you do not need to worry about that.

For the bug, the reason multiple dialogs were being rendered is it is in the map. I didn't catch that before in the last PR. Recall it was originally placed there to bind the individual workflow ids to the click hander, but that's not being done anymore. Instead of redundantly setting the state with the default workflow id, pull the rendering of the dialog outside of the map of the workflows.

jelliotartz · 2017-11-06T22:35:22Z

PR Update

Added unit tests for all of the event handlers, except for handleToggle
RFC: I've been stuck on how to test the call of handleToggle for a while now - pretty sure I'm missing something in the syntax, but can't figure it out.
- I'm getting AssertError: expected handleToggle to be called once but was called 0 times
- My hunch is that the issue is a syntax error here, but I could be off base on that.
- Additionally, this event handler is different in that it's an event that is passed to a custom component, which then calls it via an onChange event on its input - I've been reading links from Sinon's docs for how to stub complex objects, but so far no luck applying their solutions to this scenario.
- Would appreciate suggestions on how to test the handleToggle event handler :)

srallen

About the failing test, check out enzyme's docs on simulate. The second gotcha gives a clue:

Even though the name would imply this simulates an actual event, .simulate() will in fact target the component's prop based on the event you give it. For example, .simulate('click') will actually get the onClick prop and call it.

The test is currently finding the last shallow rendered WorkflowToggle and attempting to simulate the click event, which as enzyme describes, finds the onClick prop and calls it. WorkflowToggle doesn't have an onClick prop. The prop handleToggle is passed down to the child which does have an onClick prop. So, this event should be unit tested on WorkflowToggle. Testing the prop passing down to WorkflowToggle and that the class method handleToggle is called would be an integration test.

Also, just an FYI, the event variable isn't defined, so even if you were going to write an integration test, an undefined variable is being passed into the simulated event.

srallen · 2017-11-07T16:30:17Z

app/pages/admin/project-status.spec.js

+
+    wrapper = shallow(<ProjectStatus />);
+
+    wrapper.setState({


This doesn't need to happen in the before. You're setting state in your tests already for the specific condition that's being tested.

srallen · 2017-11-07T16:31:15Z

app/pages/admin/project-status.spec.js

+
+  describe('custom components', () => {
+    it('LoadingIndicator renders if project is not in state', () => {
+      wrapper.setState({ project: null });


This doesn't need to happen once you remove the setState out of the before block. You do want to test what is rendered for the initial state. There should also be a unit test for when the workflows state is null (the initial state for workflows)

srallen · 2017-11-07T16:32:07Z

app/pages/admin/project-status.spec.js

+      assert.equal(loadingIndicator.length, 1);
+    });
+
+    it('LoadingIndicator does not render if project is in state', () => {


Perhaps put the unit tests that rely upon there being a project in the component state in its own describe block.

srallen · 2017-11-07T16:32:37Z

app/pages/admin/project-status.spec.js

+    onChangeWorkflowLevelSpy = sinon.stub(ProjectStatus.prototype, 'onChangeWorkflowLevel');
+    handleDialogCancelSpy = sinon.stub(ProjectStatus.prototype, 'handleDialogCancel');
+    handleDialogSuccessSpy = sinon.stub(ProjectStatus.prototype, 'handleDialogSuccess');
+    handleToggleSpy = sinon.stub(ProjectStatus.prototype, 'handleToggle');


Let's be consistent with variable names. If they're stubs, then name the variables stubs.

jelliotartz · 2017-11-07T19:40:03Z

Thanks, yeah, I was thinking perhaps the issue was actually that I was still getting integration and unit tests mixed up - thanks for clearing that up.

Tests now refactored into describe blocks that set state in a rational way - start with the initial null states for project and workflow, test for loading indicator, then add the project mock object, test what should/shouldn't be there, then add workflows, and appropriate unit tests.

jelliotartz · 2017-11-07T19:56:32Z

app/pages/admin/project-status.spec.js

      const versionListComponent = wrapper.find('VersionList');
      assert.equal(versionListComponent.length, 1);
    });
+
+    it('renders a no workflows found message when no workflow is in component state', () => {
+      const noWorkflowsMessage = wrapper.find('div[children="No workflows found"]');


I was able to target this element using a solution suggested here, but, as one person comments, this seems tightly coupled to the implementation - would it be better to move this text to a Translate component, and then test whether what is rendered matches what is in the Translate component's content prop? (something more like this)

We don't need translations here. I'm of the opinion that finding specific DOM elements for a unit test will always be coupled with the implementation. It's unavoidable, however, I would avoid using a prop that is internal to the React API as well as using a selector that looks like a CSS selector when it's not. Here's how I'd write this test and the DOM traversal:

const noWorkflowsMessage = wrapper.find('.project-status__section').at(1).children().find('div'); assert.equal(noWorkflowsMessage.text(), 'No workflows found');

It also better matches the description of the test, which is that it renders a specific message.

- Adds basic page layout tests. - Adds tests for workflow settings. - Tests for modal dialog for toggle default workflow not working.

jelliotartz · 2017-11-08T17:36:38Z

Thanks, I'll keep that in mind going forward :)

Just rebased this against master.

srallen reviewed Nov 1, 2017

View reviewed changes

srallen self-assigned this Nov 1, 2017

jelliotartz commented Nov 2, 2017

View reviewed changes

srallen suggested changes Nov 3, 2017

View reviewed changes

srallen suggested changes Nov 7, 2017

View reviewed changes

jelliotartz changed the title ~~[WIP/RFC]: ProjectStatus unit tests~~ ProjectStatus unit tests Nov 7, 2017

jelliotartz force-pushed the project-status-unit-tests branch from ed40f5b to ba00182 Compare November 7, 2017 19:42

jelliotartz commented Nov 7, 2017

View reviewed changes

jelliotartz added 9 commits November 8, 2017 11:34

WIP: unit tests for ProjectStatus

593a94a

- Adds basic page layout tests. - Adds tests for workflow settings. - Tests for modal dialog for toggle default workflow not working.

Refactor ProjectStatus unit tests.

1c4f3a5

Refactor tests.

563dd6a

Bug fix: only render one modal for workflow default dialog.

c00f11e

Update WorkflowDefaultDialog test post bug fix.

f5c8892

Move default workflow modal outside workflow map.

37f83b3

WIP: Add event handler unit tests.

75fb02a

Refactor tests.

4f8e5e7

Refactor unit test for no workflows message.

6d7254b

jelliotartz force-pushed the project-status-unit-tests branch from 81f7192 to 6d7254b Compare November 8, 2017 17:34

srallen approved these changes Nov 8, 2017

View reviewed changes

srallen merged commit d6ece84 into master Nov 8, 2017

srallen deleted the project-status-unit-tests branch November 8, 2017 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ProjectStatus unit tests #4127

ProjectStatus unit tests #4127

jelliotartz commented Oct 30, 2017

srallen left a comment

jelliotartz commented Nov 1, 2017 •

edited

Loading

srallen commented Nov 2, 2017

jelliotartz Nov 2, 2017 •

edited

Loading

jelliotartz commented Nov 2, 2017

srallen left a comment

jelliotartz commented Nov 6, 2017 •

edited

Loading

srallen left a comment

srallen Nov 7, 2017

srallen Nov 7, 2017

srallen Nov 7, 2017

srallen Nov 7, 2017

jelliotartz commented Nov 7, 2017

jelliotartz Nov 7, 2017

srallen Nov 8, 2017

jelliotartz commented Nov 8, 2017

ProjectStatus unit tests #4127

ProjectStatus unit tests #4127

Conversation

jelliotartz commented Oct 30, 2017

Describe your changes

Review Checklist

Optional

srallen left a comment

Choose a reason for hiding this comment

jelliotartz commented Nov 1, 2017 • edited Loading

PR Update

srallen commented Nov 2, 2017

jelliotartz Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

jelliotartz commented Nov 2, 2017

PR Update

srallen left a comment

Choose a reason for hiding this comment

jelliotartz commented Nov 6, 2017 • edited Loading

PR Update

srallen left a comment

Choose a reason for hiding this comment

srallen Nov 7, 2017

Choose a reason for hiding this comment

srallen Nov 7, 2017

Choose a reason for hiding this comment

srallen Nov 7, 2017

Choose a reason for hiding this comment

srallen Nov 7, 2017

Choose a reason for hiding this comment

jelliotartz commented Nov 7, 2017

jelliotartz Nov 7, 2017

Choose a reason for hiding this comment

srallen Nov 8, 2017

Choose a reason for hiding this comment

jelliotartz commented Nov 8, 2017

jelliotartz commented Nov 1, 2017 •

edited

Loading

jelliotartz Nov 2, 2017 •

edited

Loading

jelliotartz commented Nov 6, 2017 •

edited

Loading