-
Notifications
You must be signed in to change notification settings - Fork 60
Conversation
.eslintrc
Outdated
@@ -14,6 +14,7 @@ | |||
"ipc": true, | |||
"PRODUCTION": true, | |||
"TEST": true, | |||
"step": true |
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.
is this necessary if you use import { step } from 'mocha-steps';
?
escape: 27, | ||
}; | ||
|
||
const store = prepareStore({ |
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 should happen inside a Given
step
import * as delegateApi from '../../utils/api/delegate'; | ||
import VoteDialog from './index'; | ||
|
||
const delegates = [ |
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.
Why other data object declarations (e.g. const realAccount
) are inside describe
and this declaration is outside? All const
declarations should be either inside or outside describe
(IMO inside).
unconfirmedBalance: '0', | ||
}; | ||
|
||
const peers = { |
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.
Lines 49-83 can be much simpler with use of 'lisk-js': const peers = Lisk.api({ testnet: true });
data: peers, | ||
type: actionTypes.activePeerSet, | ||
}); | ||
const wrapper = mount(renderWithRouter(VoteDialog, store)); |
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.
mount
should be inside a Given
step
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.
No, it shouldn't. because there is one component which will be test in each step and we don't need to mount it before each steps. if we do that , we will lose the last state.
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.
It sounds like you confuse Given
step with beforeEach
block. What you are saying is true for beforeEach
block, but not for Given
step.
Of course, you need to declare let wrapper
outside of the step so that you can access it in other steps.
wrapper.find('.vote-list span').last().simulate('click'); | ||
wrapper.update(); | ||
expect(wrapper.find('.primary-button button').props().disabled).to.be.equal(true); | ||
clock.restore(); |
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.
clock.restore();
should be in afterEach
block, because it should be called even if some of the steps fail.
Currently if one of the previous steps fail, then clock.restore()
is not called and other tests outside this file might be affected.
expect(wrapper.find('.primary-button button').props().disabled).to.be.equal(false); | ||
}); | ||
|
||
step('When user deletes all items form voteList confirm button should become disabled', () => { |
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 step should in Given-When-Then become two steps: 'When user deletes all items form voteList'
'Then confirm button should become disabled'
expect(store.getState().peers).to.be.an('Object'); | ||
}); | ||
|
||
step('when he doesn\'t vote to any delegates confirm button should be disabled', () => { |
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 should IMO say just 'Then confirm button is disabled'
.
Also, we should use gender-neutral language. So either 'he/she'
, or even better 'I'
to be consistent with e2e tests.
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.
I will replace he with 'user' to make it more general.
toFake: ['setTimeout', 'clearTimeout', 'Date'], | ||
}); | ||
|
||
step('user is login in', () => { |
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.
Is this Given, When or Then?
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 is the initialize step . we don't need 'when' or 'then' in this step description.
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.
Given
is the initialization step.
See: https://martinfowler.com/bliki/GivenWhenThen.html
The given part describes the state of the world before you begin the behavior you're specifying in this scenario. You can think of it as the pre-conditions to the test.
})); | ||
expect(store.getState().voting.refresh).to.be.equal(true); | ||
wrapper.update(); | ||
const voteAutocompleteApiStub = sinon.stub(delegateApi, 'voteAutocomplete'); |
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.
IMO delegateApi
is too early in the call chain. We should mock something further down. Either lisk-js or http call library (popsicle).
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.
Good job @yasharAyari
What was the problem?
We had a lot of unnecessary e2e test that makes our build process to slow.
How did I fix it?
I replaced those e2e test with integration test
How to test it?
Run npm test
Review checklist