-
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 18 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,72 @@ | ||
import {fromJS} from 'immutable'; | ||
|
||
import { | ||
projectRestoredFromLastSession as projectRestoredFromLastSessionAction, | ||
snapshotImported as snapshotImportedAction, | ||
} from '../../actions/clients'; | ||
import {validatedSource} from '../../actions/errors'; | ||
import { | ||
changeCurrentProject as changeCurrentProjectAction, | ||
gistImported as gistImportedAction, | ||
toggleLibrary as toggleLibraryAction, | ||
} from '../../actions/projects'; | ||
import validateCurrentProject from '../validateCurrentProject'; | ||
|
||
import {makeTestLogic} from './helpers'; | ||
|
||
jest.mock('../../analyzers'); | ||
|
||
const mockCssValidationErrors = [ | ||
{ | ||
text: 'You have a starting { but no ending } to go with it.', | ||
}, | ||
]; | ||
|
||
const mockHtmlValidationErrors = [ | ||
{ | ||
text: 'Closing tag missing', | ||
}, | ||
]; | ||
|
||
jest.mock('../../util/retryingFailedImports', () => | ||
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. Can we mock the |
||
jest.fn(() => ({ | ||
css: jest.fn(() => mockCssValidationErrors), | ||
html: jest.fn(() => mockHtmlValidationErrors), | ||
javascript: jest.fn(() => []), | ||
})), | ||
); | ||
|
||
test('should validate current project', async () => { | ||
const state = fromJS({ | ||
currentProject: {projectKey: '123'}, | ||
projects: { | ||
123: { | ||
sources: { | ||
html: '', | ||
css: '', | ||
javascript: '', | ||
}, | ||
}, | ||
}, | ||
}); | ||
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. Generally when we’re testing against a particular starting state, I think it’s best to generate that state by running a series of actions through a reducer, rather than hand-rolling it—that way if we make changes to the state shape, updating the reducers will automatically update all the tests. Here’s a good example from another logic test. |
||
const testLogic = makeTestLogic(validateCurrentProject); | ||
|
||
[ | ||
changeCurrentProjectAction, | ||
gistImportedAction, | ||
snapshotImportedAction, | ||
projectRestoredFromLastSessionAction, | ||
toggleLibraryAction, | ||
].forEach(async action => { | ||
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 NITPICK: Why not 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. sorry, totally missed this comment |
||
const dispatch = await testLogic(action('123'), { | ||
state, | ||
}); | ||
|
||
expect(dispatch).toHaveBeenCalledWith( | ||
validatedSource('html', mockHtmlValidationErrors), | ||
); | ||
expect(dispatch).toHaveBeenCalledWith( | ||
validatedSource('css', mockCssValidationErrors), | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import {fromJS} from 'immutable'; | ||
|
||
import {validatedSource} from '../../actions/errors'; | ||
import {updateProjectSource as updateProjectSourceAction} from '../../actions/projects'; | ||
import validateProjectOnChange from '../validateProjectOnChange'; | ||
|
||
import {makeTestLogic} from './helpers'; | ||
|
||
jest.mock('../../analyzers'); | ||
|
||
const mockCssValidationErrors = [ | ||
{ | ||
text: 'You have a starting { but no ending } to go with it.', | ||
}, | ||
]; | ||
|
||
jest.mock('../../util/retryingFailedImports', () => | ||
jest.fn(() => ({ | ||
css: jest.fn(() => mockCssValidationErrors), | ||
html: jest.fn(() => []), | ||
javascript: jest.fn(() => []), | ||
})), | ||
); | ||
|
||
test('should validate project on change', async () => { | ||
const state = fromJS({ | ||
currentProject: {projectKey: '123'}, | ||
projects: { | ||
123: { | ||
sources: { | ||
html: '', | ||
css: 'div {', | ||
javascript: '', | ||
}, | ||
}, | ||
}, | ||
}); | ||
|
||
const testLogic = makeTestLogic(validateProjectOnChange); | ||
const dispatch = await testLogic( | ||
updateProjectSourceAction('123', 'css', 'div {'), | ||
{state}, | ||
); | ||
|
||
expect(dispatch).toHaveBeenCalledWith( | ||
validatedSource('css', mockCssValidationErrors), | ||
); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import {validatedSource} from '../../actions/errors'; | ||
import {getCurrentProject} from '../../selectors'; | ||
import retryingFailedImports from '../../util/retryingFailedImports'; | ||
|
||
function importValidations() { | ||
return retryingFailedImports(() => | ||
import( | ||
/* webpackChunkName: "mainAsync" */ | ||
'../../validations' | ||
), | ||
); | ||
} | ||
|
||
export default async function validateSource( | ||
{language, source, projectAttributes}, | ||
dispatch, | ||
getState, | ||
) { | ||
const validations = await importValidations(); | ||
const validate = validations[language]; | ||
const validationErrors = await validate(source, projectAttributes); | ||
const state = getState(); | ||
const currentProject = getCurrentProject(state); | ||
if (currentProject.sources[language] === source) { | ||
dispatch(validatedSource(language, validationErrors)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import map from 'lodash-es/map'; | ||
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', | ||
], | ||
latest: true, | ||
async process({getState}, dispatch, done) { | ||
const state = getState(); | ||
const currentProject = getCurrentProject(state); | ||
const analyzer = new Analyzer(currentProject); | ||
|
||
const validatePromises = map( | ||
Reflect.ownKeys(currentProject.sources), | ||
language => | ||
validateSource( | ||
{ | ||
language, | ||
source: currentProject.sources[language], | ||
projectAttributes: analyzer, | ||
}, | ||
dispatch, | ||
getState, | ||
), | ||
); | ||
|
||
await Promise.all(validatePromises); | ||
done(); | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
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, | ||
getState, | ||
); | ||
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.
This seems like a pretty good candidate for a factory? Generally any time we need sample data to use in a test that conforms to a particular well-defined shape, a factory is the move (since it’s then reusable and rosie makes it super-easy to customize the generated objects for the needs of particular tests)