search

chartjs / chartjs-plugin-annotation

Annotation plugin for Chart.js
MIT License
607 stars 328 forks source link

Line: change event tests to inRange tests #657

Closed stockiNail closed 2 years ago

stockiNail commented 2 years ago

This PR remove events test cases with inRange method tests.

kurkle commented 2 years ago

Coverage decreased (-1.08%) to 95.274%

stockiNail commented 2 years ago

Looks ok, but I don't really like that we have to be one pixel inside the element before the tests work. Also should test different borderWidths of the line (1 for the other edge case, or should it actually be 0.1?). Also without label.

Without 1 inRange return false. I need to go in deep and debug it. I have the feeling it's related to intersect function and the epsilon. Let me debug it and I'll come back with the result.

stockiNail commented 2 years ago

Coverage decreased (-1.08%) to 95.274%

It's correct because the events part has been removed but it's present in the other PR.

kurkle commented 2 years ago

Looks ok, but I don't really like that we have to be one pixel inside the element before the tests work. Also should test different borderWidths of the line (1 for the other edge case, or should it actually be 0.1?). Also without label.

Without 1 inRange return false. I need to go in deep and debug it. I have the feeling it's related to intersect function and the epsilon. Let me debug it and I'll come back with the result.

I think it would be fine if the test passes with borderWidth: 1. If it does not, then I think its worth taking a closer look.

stockiNail commented 2 years ago

Looks ok, but I don't really like that we have to be one pixel inside the element before the tests work. Also should test different borderWidths of the line (1 for the other edge case, or should it actually be 0.1?). Also without label.

Without 1 inRange return false. I need to go in deep and debug it. I have the feeling it's related to intersect function and the epsilon. Let me debug it and I'll come back with the result.

I think it would be fine if the test passes with borderWidth: 1. If it does not, then I think its worth taking a closer look.

I found the bug in intersect... Let me test again (with borderWidth: 1 and others)

stockiNail commented 2 years ago

Ok the bug was here:

https://github.com/chartjs/chartjs-plugin-annotation/blob/c04e02ef2ff171cd878a308a34154c19632c00af/src/types/line.js#L66

It must be:

return (sqr(x - xx) + sqr(y - yy)) <= epsilon;
stockiNail commented 2 years ago

I'm committing the changes and then I'll rebase it.

kurkle commented 2 years ago

I think after this we are good to go with 1.3.0?

So the version bump would be next. The we could give it a week or 2 before starting to merge v2 things. The week or 2 so there is room to fix any new bugs found in 1.3 without creating any additional branches and stuff.

stockiNail commented 2 years ago

I think after this we are good to go with 1.3.0?

So the version bump would be next. The we could give it a week or 2 before starting to merge v2 things. The week or 2 so there is room to fix any new bugs found in 1.3 without creating any additional branches and stuff.

Completely agree with you!

stockiNail commented 2 years ago

Coveralls - ubuntu-latest-firefox — Coverage decreased (-0.0%) to 95.947%