New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP][web] Coverage improve #2322

Merged
merged 7 commits into from May 10, 2017

Conversation

Projects
None yet
2 participants
@MatthewShao
Contributor

MatthewShao commented May 7, 2017

Here's the brief road map about adding tests for our JavaScript code.

  • Clear up the code: remove jest.unmock()
  • Add full coverage to the files in the path we have touched, for example js/ducks, js/ducks/ui
    • js/ducks/index.js
    • js/ducks/ui/index.js
    • js/ducks/ui/keyboard.js

Next step: Add tests for components in the .jsx files.

The reason why we need to remove jest.unmock has been discussed here, I think we should keep the automocking disabled and manually mock the modules if necessary.

[web] Clear up jest.unmock()
Automocking is no longer enable by default, so we don't need to unmock
modules manually.
@mhils

This comment has been minimized.

Show comment
Hide comment
@mhils

mhils May 7, 2017

Member

Sounds good! You can merge this right away if you want, but also feel free to follow through with the other points in this PR if you prefer that! 😃

Member

mhils commented May 7, 2017

Sounds good! You can merge this right away if you want, but also feel free to follow through with the other points in this PR if you prefer that! 😃

@mhils

mhils approved these changes May 7, 2017

@mhils

mhils approved these changes May 8, 2017

@MatthewShao

This comment has been minimized.

Show comment
Hide comment
@MatthewShao

MatthewShao May 9, 2017

Contributor

Let me explain these tests a bit. We bound many actions to the keyboard, which have different mechanism. It took me more time than expected to figure out a solution to cover them up...

  1. For the actions which change the state by invoking reducer, (for example cursor movement), we can create a mock store by using redux-mock-store, and assert the actions pass to the reducer.
  2. For the actions which simply invoke fetchApi, we have to introduce the real store object because the mocked one does not actually dispatch things to the reducer. Then we assert the arguments pass to the fetchApi.

If these new tests are good enough, I think it's time to move on to the components parts. I hope this PR can be merged before we touch those paths, because we are going to experience a dramatic drop on the coverage rate. Hold on for the drop! 😄

Contributor

MatthewShao commented May 9, 2017

Let me explain these tests a bit. We bound many actions to the keyboard, which have different mechanism. It took me more time than expected to figure out a solution to cover them up...

  1. For the actions which change the state by invoking reducer, (for example cursor movement), we can create a mock store by using redux-mock-store, and assert the actions pass to the reducer.
  2. For the actions which simply invoke fetchApi, we have to introduce the real store object because the mocked one does not actually dispatch things to the reducer. Then we assert the arguments pass to the fetchApi.

If these new tests are good enough, I think it's time to move on to the components parts. I hope this PR can be merged before we touch those paths, because we are going to experience a dramatic drop on the coverage rate. Hold on for the drop! 😄

@mhils

Looks good overall. Minor comments below. :)

MatthewShao and others added some commits May 9, 2017

@mhils

mhils approved these changes May 9, 2017

@MatthewShao MatthewShao merged commit cafa094 into mitmproxy:master May 10, 2017

4 checks passed

codecov/patch Coverage not affected when comparing fdec51d...537d5fa
Details
codecov/project 87.85% (+5.77%) compared to fdec51d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment