Tie-break closest point in rendering order and allow overriding more methods in DataPointTooltip
#457
Conversation
|
Hm, apparently some of the CI does not run automatically, otherwise I would have been interested to see whether the test fails there too. Following my thoughts in #454, the Codacy suggestion is not something I'd like to action 🤔 |
Codecov Report
@@ Coverage Diff @@
## master #457 +/- ##
============================================
+ Coverage 52.91% 53.48% +0.56%
- Complexity 7268 7469 +201
============================================
Files 383 389 +6
Lines 40349 40925 +576
Branches 6512 6598 +86
============================================
+ Hits 21349 21887 +538
- Misses 17461 17464 +3
- Partials 1539 1574 +35
Continue to review full report at Codecov.
|
|
Thanks for the PR, looks very good, that case just wasn't considered when the code was written. I don't see how this change could have negative impacts generally. Regarding the test error, we do not have much experience with running TestFX on Windows. You could try to run them from your IDE in non-headless mode to see if it works when using the native toolkit instead of of the monocle software renderer. The error doesn't really look like it is related to the changes you added. Locally the tests pass for me and i also started the CI run. The failure there is only due to CI configuration for external PRs (it is missing a secret for submitting the coverage report). Making the tests also assert that the order is correct would be nice, but if you cannot get the testFx tests running locally, i guess we could add that ourselves, your suggested approach seems reasonable. Regarding the codacy suggestion, I would say that inequality operations on floats are generally not a problem. From a correctness pov both statements are equivalent and I would tend to agree with Codacy on readability. |
|
Swapped the comparison and added the test anyways - if CI can run everything and does not have the issue I have on my machine, then it should be able to validate that the behaviour is correct (I've not been able to get this working locally). |
Closes #454.
I got a tooltip-related test failure from
testThatTooltipIsShownon check-out, which persists after my small changes:Trace
To be honest, I don't have much of an idea what's happening there. Apart from the error, do you want explicit testing for the now-fixed tie-breaking behaviour or do you prefer to leave this open for change in the future? In the former case, I think it is sufficient to duplicate one of the data sets in the existing test exactly and test that the current tooltips are unchanged regardless, though due to the above error I couldn't actually run that test.