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

Updated controller.scatter.test.js to test default tooltip callbacks #5967

Conversation

darless
Copy link
Contributor

@darless darless commented Jan 8, 2019

What changed

  • This moves the mouse over the drawn point and verifies that there is
    no title in the tooltip and that the body contains expected.

Reason for change

  • Initially implemented because I saw lacking code coverage for the default tooltip callbacks.

Testing

  • Ran gulp test which passed both lint and unittests.
  • Ran gulp unittest --coverage to verify that the associated lines for the controller.scatter.js are now covered.

@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 8, 2019
@benmccann
Copy link
Contributor

@Madrussian did you see test/specs/core.tooltip.tests.js? We have quite a bit of tooltip tests already and I think this is probably already covered by existing tests

I'm not sure that this test really belongs in controller.scatter.test.js. There's not really any tooltip-related code in the scatter controller. The location of the test code isn't corresponding to where the tooltip code is defined if we put it there, so I think it'd be better to add any additional tooltip test coverage to test/specs/core.tooltip.tests.js

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.

@Madrussian it totally makes sense to have it in controller.scatter.test.js since it's testing scatter defaults callbacks defined in the scatter controller. Just a minor change to make it simpler.

test/specs/controller.scatter.test.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor

But do we need to test the tooltip functionality again? Maybe we should just test that the default config matches what we expect? Like in https://github.com/chartjs/Chart.js/blob/master/test/specs/scale.time.tests.js#L54

@simonbrunel
Copy link
Member

But do we need to test the tooltip functionality again?

We are not really testing tooltip functionalities but that the default options are correctly consumed.

expect(chart.options.tooltips.callbacks.title).toEqual(jasmine.any(Function));
expect(chart.options.tooltips.callbacks.label).toEqual(jasmine.any(Function));

Testing defaults as in the time scale doesn't guarantee that we don't break the current behavior, which is an empty title and (x, y) labels. We could manually call these methods and check their results, but how the test is currently written is more robust and as simple as just checking defaults.

- This moves the mouse over the drawn point and verifies that there is
  no title in the tooltip and that the body contains expected content.
@darless darless force-pushed the controller.scatter.test-default-tooltips branch from 4ca06b3 to b11d643 Compare January 8, 2019 17:57
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.

Left one minor comment. Am ok with the content of the test

test/specs/controller.scatter.test.js Show resolved Hide resolved
@simonbrunel simonbrunel merged commit 9ecae7c into chartjs:master Jan 9, 2019
@simonbrunel
Copy link
Member

Thanks @Madrussian

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.

5 participants