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] Add tests for js/components/Header #2348

Merged
merged 15 commits into from May 29, 2017

Conversation

Projects
None yet
2 participants
@MatthewShao
Contributor

MatthewShao commented May 23, 2017

  • FilterDocs.jsx
  • ConnectionIndicator.jsx
  • FIleMenu.jsx
  • FIlterInput.jsx
  • FlowMenu.jsx
  • MenuToggle.jsx
  • OptionMenu.jsx
  • MainMenu.jsx
@MatthewShao

This comment has been minimized.

Show comment
Hide comment
@MatthewShao

MatthewShao May 23, 2017

Contributor

@mhils @cle1000 , I met some trouble here. To make the test go into the FilterDocs.xhr logic, I introduce a dependency jest-fetch-mock. But we still have two line missed.

  1. https://github.com/mitmproxy/mitmproxy/blob/master/web/src/js/components/Header/FilterDocs.jsx#L20, even with jest-fetch-mock, I could not find a way to mock the connection error, some mock response with error status code could not trigger the catch method.
  2. https://github.com/mitmproxy/mitmproxy/blob/master/web/src/js/components/Header/FilterDocs.jsx#L40 , it turns out that the render result is <i className="fa fa-spinner fa-spin"></i>, instead of the docs table.

It's kind of tricky to cover up the lines here, and I notice that there's a TODO saying: move to redux in this file, so I wonder should we do this right now to add up the testability, if so, please give me some guidance.

Contributor

MatthewShao commented May 23, 2017

@mhils @cle1000 , I met some trouble here. To make the test go into the FilterDocs.xhr logic, I introduce a dependency jest-fetch-mock. But we still have two line missed.

  1. https://github.com/mitmproxy/mitmproxy/blob/master/web/src/js/components/Header/FilterDocs.jsx#L20, even with jest-fetch-mock, I could not find a way to mock the connection error, some mock response with error status code could not trigger the catch method.
  2. https://github.com/mitmproxy/mitmproxy/blob/master/web/src/js/components/Header/FilterDocs.jsx#L40 , it turns out that the render result is <i className="fa fa-spinner fa-spin"></i>, instead of the docs table.

It's kind of tricky to cover up the lines here, and I notice that there's a TODO saying: move to redux in this file, so I wonder should we do this right now to add up the testability, if so, please give me some guidance.

@mhils

mhils approved these changes May 26, 2017

Show outdated Hide outdated web/package.json
@mhils

+9% web coverage, fantastic! 😍

Minor comments below.

const InterceptInput = connect(
state => ({
value: state.settings.intercept || '',
placeholder: 'Intercept',
type: 'pause',
color: 'hsl(208, 56%, 53%)'
}),
{ onChange: intercept => updateSettings({ intercept }) }
{ onChange: setIntercept }

This comment has been minimized.

@mhils

mhils May 29, 2017

Member

Why this?

@mhils

mhils May 29, 2017

Member

Why this?

This comment has been minimized.

@MatthewShao

MatthewShao May 29, 2017

Contributor

Here we need to extract the anonymous function, otherwise this line could not be covered.

@MatthewShao

MatthewShao May 29, 2017

Contributor

Here we need to extract the anonymous function, otherwise this line could not be covered.

export function TStore(){
let tflow = new TFlow()
return mockStore({
ui: {

This comment has been minimized.

@mhils

mhils May 29, 2017

Member

Any particular reason why we don't use the respective reducers here? This currently is a good candidate where we could get out of sync.

@mhils

mhils May 29, 2017

Member

Any particular reason why we don't use the respective reducers here? This currently is a good candidate where we could get out of sync.

This comment has been minimized.

@MatthewShao

MatthewShao May 29, 2017

Contributor

If we use the reducers, it will look like this:

export function TStore(){
    let tflow = new TFlow(),
        uiState = null,
        setttingState = null
    uiState = reduceUI(undefined, UIActions.setContentViewDescription('foo'))
    uiState = reduceUI(uiState, UIActions.setContent(['foo', 'bar']))
    uiState = reduceUI(uiState, UIActions.startEdit(tflow))
    setttingState = reduceSetting(setttingState, {type: SettingActions.UPDATE, data: { contentViews: ['Auto', 'Raw', 'Text']}})
    setttingState = reduceSetting(setttingState, {type: SettingActions.UPDATE, data: { anticache: true }})
    setttingState = reduceSetting(setttingState, {type: SettingActions.UPDATE, data: { anticomp: false }})
    return mockStore({
        ui: uiState,
        settings: settingState
        })
}

Basically, we need to pass a bunch of actions to the reducers to get the state object, it's a little bit redundant. Would it be more straightforward and clear to define the state object statically? In that way we could see the structure of the store object and modify some value easily.

@MatthewShao

MatthewShao May 29, 2017

Contributor

If we use the reducers, it will look like this:

export function TStore(){
    let tflow = new TFlow(),
        uiState = null,
        setttingState = null
    uiState = reduceUI(undefined, UIActions.setContentViewDescription('foo'))
    uiState = reduceUI(uiState, UIActions.setContent(['foo', 'bar']))
    uiState = reduceUI(uiState, UIActions.startEdit(tflow))
    setttingState = reduceSetting(setttingState, {type: SettingActions.UPDATE, data: { contentViews: ['Auto', 'Raw', 'Text']}})
    setttingState = reduceSetting(setttingState, {type: SettingActions.UPDATE, data: { anticache: true }})
    setttingState = reduceSetting(setttingState, {type: SettingActions.UPDATE, data: { anticomp: false }})
    return mockStore({
        ui: uiState,
        settings: settingState
        })
}

Basically, we need to pass a bunch of actions to the reducers to get the state object, it's a little bit redundant. Would it be more straightforward and clear to define the state object statically? In that way we could see the structure of the store object and modify some value easily.

This comment has been minimized.

@mhils

mhils May 29, 2017

Member

Ok, your argument on seeing the structure is quite convincing. Let's continue as you proposed, if we see that this causes consistency issues in the long run, we could still change it. :)

@mhils

mhils May 29, 2017

Member

Ok, your argument on seeing the structure is quite convincing. Let's continue as you proposed, if we see that this causes consistency issues in the long run, we could still change it. :)

Show outdated Hide outdated web/src/js/__tests__/components/Header/FilterDocsSpec.js
Show outdated Hide outdated web/src/js/__tests__/components/Header/FilterInputSpec.js

MatthewShao added some commits May 29, 2017

@mhils mhils merged commit ec7d7c9 into mitmproxy:master May 29, 2017

4 checks passed

codecov/patch 100% of diff hit (target 84.65%)
Details
codecov/project 85.6% (+0.94%) compared to e7f7a60
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