Improve controller test stability

[andig]

Follow-up to #4694 (comment)

@simonbrunel your suggestion seems to work- no regression with the bar tests. Please have a careful look at doughnut and polar tests- I'm not sure, not having experience with those charts, that the value changes are plausible.

[simonbrunel]

That's right that the doughnut and polar expectations looks weird (0 for innerRadius / outerRadius). Did you try to put a debugger; before the expectations to see what the chart looks like?

While working on the chartjs-plugin-datalabels, I noticed that setting Chart.defaults.global.label: false didn't work with doughnut charts, I need to set Chart.defaults.global.label.display: false so might be the same issue. Also, could be even better to completely turn off the plugins instead:

options: {
  plugins: {
    legend: false,
    title: false
  }
}

[etimberg]

Yeah, the 0 size looks wrong :/

[andig]

While working on the chartjs-plugin-datalabels, I noticed that setting Chart.defaults.global.label: false didn't work with doughnut charts, I need to set Chart.defaults.global.label.display: false so might be the same issue. Also, could be even better to completely turn off the plugins instead:

Did just a quick test now (and thanks for the debugger; hint, didn't know about it)- with either global.title = false or global.legend = false even the most simple doughnot test (create arcs) doesn't run.

[andig]

@simonbrunel should be complete now.

[etimberg]

I'm guessing the error with the doughnut and polar area charts is due to

Chart.js/src/helpers/helpers.core.js

Line 189 in 4c763bf

target[key] = helpers.clone(sval);

because the defaults for those chart types have legend options. This means that they would still get applied but the global defaults would be wiped out so it would have invalid options

[simonbrunel]

@etimberg good catch, not sure if it's fixable to allow user to do Chart.defaults.global.legend: false (a workaround is to disable plugins instead).

The last failure was caused by the merge in afterAll: Chart.defaults.global.plugin.legend is initially undefined (so not present in the cloned this._defaults). When merging back the saved defaults, Chart.defaults.global.plugin.legend remained unchanged (false) for other unit tests.

It's not possible to fully change the Chart.defaults because internal code will still use the previous reference, so can't simply do Chart.defaults = this._defaults in afterAll. One solution (a bit tricky) consists to use a custom merger that returns the previous overwritten values:

this._defaults = jasmine.overrideDefaults({
  global: {
    plugins: {
      legend: false,
      title: false
    }
  }
});

/*
this._defaults === {
  global: {
    plugins: {
      legend: undefined,
      title: undefined
    }
  }
}
*/

Manipulating defaults in unit tests is not safe since it impacts all specs. If you prefer to not touch the defaults, then we will need to inject that options for every acquireChart (more repetitive work but maybe cleaner?!).

[andig]

You've lost me in the discussion but thanks ok since you can perform the additional updates as desired.

[simonbrunel]

I finally think it's better to not rely on defaults to globally disable plugins / hide scales but instead inject these options while acquiring charts. Altering global defaults is not safe and is error prone since we don't really know what are the current options for every tests.

I'm going to update the PR with these changes.

[simonbrunel]

@etimberg @andig can you guys have another look over this PR?

[andig]

I‘m not sure. May of the tests af simply changed order of chart specs now. Axes are often no longer hidden. Although that might not be required it would make the tests easier to update on future changes?

[simonbrunel]

I didn't understand your first sentence: I simply removed the beforeAll / afterAll and locally disable plugins / hide scale for failing tests only. Tests that don't check (pixel) position actually don't need these tweaks.

[andig]

I meant these occasions https://github.com/chartjs/Chart.js/pull/4698/files#diff-b4730e5a7a6104600b439e9b30fc34afR826 and following. It may be better going back to the first commit, cleaning these out and force-push?

[simonbrunel]

I changed it to keep the order consistent between all tests. Not sure what the issue with changing the properties order, but up to you if you want to revert it, I will not have time to iterate more on this PR.

[andig]

No, fine with me. Just thought you might want to keep the changes minimal and didn‘t notice since th initial change resulted from my commit. No problem with cutting the losses on this one :)