Skip to content
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

Merged
merged 1 commit into from Aug 24, 2017
Merged

Make bar tests more stable #4694

merged 1 commit into from Aug 24, 2017

Conversation

andig
Copy link
Contributor

@andig andig commented Aug 24, 2017

Follow-up to #4686

My recipe for doing this semi-automatic is this:

  • use SublimeText or Atom that support multi-selection via Cmd-D
  • extract all line numbers, failures and expectations using regex (?s)Expected ([0-9.]+)( to be close to pixel (\d+)(.*? )at .*?test/specs/controller.bar.tests.js:)(\d+):(\d+)
  • copy to new file and replace same regex by \5 \3 \1 - this gives line number, current expectation, actual value
  • then simply step through the lines, multi-select and replace across the file

@@ -823,10 +887,10 @@ describe('Bar controller tests', function() {
var meta0 = chart.getDatasetMeta(0);

[
{b: 290, w: 83 / 2, x: 63, y: 161},
Copy link
Member

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Member

@simonbrunel simonbrunel left a 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

Copy link
Member

@etimberg etimberg left a 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

@etimberg etimberg merged commit 530c613 into chartjs:master Aug 24, 2017
@andig andig deleted the bar-tests-2 branch August 25, 2017 08:51
@andig
Copy link
Contributor Author

andig commented Aug 25, 2017

@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 :)

@simonbrunel
Copy link
Member

simonbrunel commented Aug 25, 2017

Sounds really good, thanks @andig :)

I guess most of the controller.*.tests.js would need this kind of update.

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 this in before/afterAll, would need to check if it's the same.

Then, only need to change expectations but not options for every tests.

yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants