-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Make bar tests more stable #4694
Conversation
@@ -823,10 +887,10 @@ describe('Bar controller tests', function() { | |||
var meta0 = chart.getDatasetMeta(0); | |||
|
|||
[ | |||
{b: 290, w: 83 / 2, x: 63, y: 161}, |
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.
Similar to a talk we had, can we keep it in this form: w: 92 / 2
(sample size / number of stacks)
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.
Fixed
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.
Thanks a lot @andig
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.
Looks great! Thanks @andig
@simonbrunel if there's another batch of tests needing cleanup feel free to let me know. I've just rebased #4646 and didn't need to touch the bar controller. The cleanup is definitely worth the efforts :) |
Sounds really good, thanks @andig :) I guess most of the I'm wondering if we could limit the changes by using global options, for example: describe('Chart.controllers.bar', function() {
beforeAll(function() {
this._defaults = Chart.helpers.clone(Chart.defaults);
Chart.helpers.merge(Chart.defaults, {
global: {
legend: false,
title: false
},
scale: {
display: false
}
});
});
afterAll(function() {
Chart.helpers.merge(Chart.defaults, this._defaults);
});
}); Not sure about Then, only need to change expectations but not options for every tests. |
Follow-up to #4686
My recipe for doing this semi-automatic is this:
(?s)Expected ([0-9.]+)( to be close to pixel (\d+)(.*? )at .*?test/specs/controller.bar.tests.js:)(\d+):(\d+)
\5 \3 \1
- this gives line number, current expectation, actual value