-
Notifications
You must be signed in to change notification settings - Fork 531
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
VueMultiSelect unit test #46
VueMultiSelect unit test #46
Conversation
…ue-form-generator into LB_noUiSlider_unit_test
…ui/vue-form-generator into LB_vueMultiSelect_unit_test
|
||
vm.$nextTick( () => { | ||
expect(model.city[0]).to.be.equal("Paris"); | ||
done(); |
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's not too precize. Please check the model.city.length to be 1
Good PR, thank you. I wrote my comments, if you fix them, I can merge it :) |
// }); | ||
|
||
it.skip("model value should be the handle value after changed", (done) => { | ||
vm.$nextTick( () => { |
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.
What's the problem with skipped 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'm following mocha documentation. It is better to skip a test than comment it.
Best practice: Use .skip() instead of commenting tests out.
As for why exactly I skipping them, there is a problem with the value.
Without vm.model = { rating: 10 };
, the slider don't move (.noUi-origin
css left
value is 0%), the value is not set at all (part of the problem come form my mistake in the schema definition, but even when fixed, there is still a problem).
When placed in a before
hook, the value is set properly.
When used in a it
statement, I need to use a setTimeout
inside the nextTick()
to get the changes.
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 check too.
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 trying to fix this test
Are you ready? Can be merge? |
Yes, it will certainly need more in the future, but this is honest. |
I fixed the noUiSlider unit test |
I based myself on fieldSelect and remove some test that made no sense for this fields (it doesn't have a value property).
I'm waiting for your comments.