-
Notifications
You must be signed in to change notification settings - Fork 138
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
Redux logic errors #1954
Redux logic errors #1954
Changes from 10 commits
4168920
c26af06
fe8d719
9bf3a0d
1227492
e40ff0f
982f9d4
e429d3b
7a6faef
4e4e64c
adfd45b
9d7a55c
1080acb
33eb932
80cf62f
9a6a751
12b20a0
99c37ca
c2be8db
5520295
23c2613
c81b993
c12991d
5a567a8
3b666dd
fbe3d0c
8826d1e
43505c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import validateSource from '../../helpers/validateSource'; | ||
import {validatedSource} from '../../../actions/errors'; | ||
|
||
const mockValidationErrors = { | ||
css: 'invalid CSS selector', | ||
}; | ||
|
||
const mockCssValidate = jest.fn(x => mockValidationErrors); | ||
|
||
jest.mock('../../../util/retryingFailedImports', () => | ||
jest.fn(x => ({ | ||
css: mockCssValidate, | ||
})), | ||
); | ||
|
||
const mockActionCreator = (language, errors) => ({ | ||
type: 'VALIDATED_SOURCE', | ||
payload: { | ||
errors: { | ||
[language]: errors[language], | ||
language, | ||
}, | ||
}, | ||
}); | ||
|
||
jest.mock('../../../actions/errors', () => ({ | ||
validatedSource: jest.fn((language, errors) => | ||
mockActionCreator(language, errors), | ||
), | ||
})); | ||
|
||
test('calls validateSource with validationErrors', async () => { | ||
const language = 'css'; | ||
const source = 'dib { color: green;}'; | ||
const projectAttributes = {}; | ||
const dispatch = jest.fn(); | ||
await validateSource({language, source, projectAttributes}, dispatch); | ||
expect(mockCssValidate).toHaveBeenCalledWith(source, projectAttributes); | ||
expect(validatedSource).toHaveBeenCalledWith(language, mockValidationErrors); | ||
const payload = validatedSource(language, mockValidationErrors); | ||
expect(dispatch).toHaveBeenCalledWith(payload); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
import validateCurrentProject from '../validateCurrentProject'; | ||
|
||
import Analyzer from '../../analyzers'; | ||
import validateSource from '../helpers/validateSource'; | ||
import {getCurrentProject} from '../../selectors'; | ||
|
||
const mockHtmlSource = '<html></html>'; | ||
const mockCssSource = 'div {}'; | ||
|
||
jest.mock('../../analyzers'); | ||
jest.mock('../helpers/validateSource'); | ||
jest.mock('../../selectors', () => ({ | ||
getCurrentProject: jest.fn(() => ({ | ||
sources: { | ||
html: mockHtmlSource, | ||
css: mockCssSource, | ||
}, | ||
})), | ||
})); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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”. |
||
|
||
const dummyState = {}; | ||
|
||
test('should validate current project', async () => { | ||
const dispatch = jest.fn(); | ||
const done = jest.fn(); | ||
await validateCurrentProject.process( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, this is a bit of a pain in the ass, but 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!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
{ | ||
getState: () => dummyState, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
}, | ||
dispatch, | ||
done, | ||
); | ||
expect(getCurrentProject).toHaveBeenCalledWith(dummyState); | ||
const analyzerPayload = getCurrentProject(dummyState); | ||
expect(Analyzer).toHaveBeenCalledWith(analyzerPayload); | ||
const analyzer = new Analyzer(analyzerPayload); | ||
expect(validateSource.mock.calls).toEqual([ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would probably use |
||
[ | ||
{ | ||
language: 'html', | ||
source: mockHtmlSource, | ||
projectAttributes: analyzer, | ||
}, | ||
dispatch, | ||
], | ||
[ | ||
{ | ||
language: 'css', | ||
source: mockCssSource, | ||
projectAttributes: analyzer, | ||
}, | ||
dispatch, | ||
], | ||
]); | ||
expect(done).toHaveBeenCalled(); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import validateProjectOnChange from '../validateProjectOnChange'; | ||
|
||
import Analyzer from '../../analyzers'; | ||
import validateSource from '../helpers/validateSource'; | ||
import {getCurrentProject} from '../../selectors'; | ||
|
||
jest.mock('../../analyzers'); | ||
jest.mock('../helpers/validateSource'); | ||
jest.mock('../../selectors', () => ({ | ||
getCurrentProject: jest.fn(x => x), | ||
})); | ||
|
||
const dummyState = {}; | ||
|
||
test('should validate project on change', async () => { | ||
const language = 'html'; | ||
const newValue = 'dummy new value'; | ||
const dispatch = jest.fn(); | ||
const done = jest.fn(); | ||
await validateProjectOnChange.process( | ||
{ | ||
getState: () => dummyState, | ||
action: {payload: {language, newValue}}, | ||
}, | ||
dispatch, | ||
done, | ||
); | ||
expect(getCurrentProject).toHaveBeenCalledWith(dummyState); | ||
const analyzerPayload = getCurrentProject(dummyState); | ||
expect(Analyzer).toHaveBeenCalledWith(analyzerPayload); | ||
expect(validateSource).toHaveBeenCalledWith( | ||
{ | ||
language, | ||
source: newValue, | ||
projectAttributes: new Analyzer(getCurrentProject(dummyState)), | ||
}, | ||
dispatch, | ||
); | ||
expect(done).toHaveBeenCalled(); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import retryingFailedImports from '../../util/retryingFailedImports'; | ||
import {validatedSource} from '../../actions/errors'; | ||
|
||
function importValidations() { | ||
return retryingFailedImports(() => | ||
import( | ||
/* webpackChunkName: "mainAsync" */ | ||
'../../validations' | ||
), | ||
); | ||
} | ||
|
||
export default async ({language, source, projectAttributes}, dispatch) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
const validations = await importValidations(); | ||
const validate = validations[language]; | ||
const validationErrors = await validate(source, projectAttributes); | ||
await dispatch(validatedSource(language, validationErrors)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How come we’re awaiting the |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import {createLogic} from 'redux-logic'; | ||
import Analyzer from '../analyzers'; | ||
import {getCurrentProject} from '../selectors'; | ||
import validateSource from './helpers/validateSource'; | ||
|
||
export default createLogic({ | ||
type: [ | ||
'CHANGE_CURRENT_PROJECT', | ||
'GIST_IMPORTED', | ||
'SNAPSHOT_IMPORTED', | ||
'PROJECT_RESTORED_FROM_LAST_SESSION', | ||
'TOGGLE_LIBRARY', | ||
], | ||
async process({getState}, dispatch, done) { | ||
const state = getState(); | ||
const currentProject = getCurrentProject(state); | ||
const analyzer = new Analyzer(currentProject); | ||
|
||
const validatePromises = []; | ||
for (const language of Reflect.ownKeys(currentProject.sources)) { | ||
const source = currentProject.sources[language]; | ||
validatePromises.push( | ||
validateSource( | ||
{language, source, projectAttributes: analyzer}, | ||
dispatch, | ||
), | ||
); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: a |
||
await Promise.all(validatePromises); | ||
done(); | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import {createLogic} from 'redux-logic'; | ||
import Analyzer from '../analyzers'; | ||
import {getCurrentProject} from '../selectors'; | ||
import validateSource from './helpers/validateSource'; | ||
|
||
export default createLogic({ | ||
type: 'UPDATE_PROJECT_SOURCE', | ||
latest: true, | ||
async process( | ||
{ | ||
getState, | ||
action: { | ||
payload: {language, newValue}, | ||
}, | ||
}, | ||
dispatch, | ||
done, | ||
) { | ||
const state = getState(); | ||
const analyzer = new Analyzer(getCurrentProject(state)); | ||
|
||
await validateSource( | ||
{language, source: newValue, projectAttributes: analyzer}, | ||
dispatch, | ||
); | ||
done(); | ||
}, | ||
}); |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thevalidateSource
helper is essentially an implementation detail ofvalidateCurrentProject
; if in the future we decided to pull all of that logic directly intovalidateCurrentProject
, 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 invalidateSource
; and remove the unit test for thevalidateSource
helper.