[Core] Improve checks in blood tests #245
Conversation
Codecov Report
@@ Coverage Diff @@
## main #245 +/- ##
=========================================
Coverage 58.52% 58.52%
Complexity 2050 2050
=========================================
Files 316 316
Lines 5119 5119
=========================================
Hits 2996 2996
Misses 2123 2123 Continue to review full report at Codecov.
|
|
Why should 100 random runs always contain every blood type? |
|
@GrahamCampbell I can't confirm that they will on every architecture, thus my comment about it not being the best possible method, but with the seeding it is at least consistent, and accurate when all are present in the run. I don't know the math off the top of my head, but there is a set size where it is improbable to the point of near impossibility that we don't get at least one of each in the result set. 100 worked on my machine so I didn't expand it beyond that -- for the sake of speed. In a perfect world we would be able to inject / overload the helper. I fear the reliance on so many static methods makes that difficult without re-architecting a large portion of the project. Basically: I went with good enough, with the hopes that the discussion around 2.0 will improve things even further. |
|
@anubisthejackle We would need to refactor all the tests in similar way which is reported in #125 and partially solved in #134 |
I agree. This PR really just attempts to reflect the comments @localheinz made on the original PR regarding these specific tests. |
bram-pkg
changed the title
|
@localheinz can you jump in here as well? Is this really the best way to do it? Overall there will never be a 100% solution with these data lists though so |
The best way to do it would be to be able to inject a randomization algorithm that we control that would allow us to force random methods to non-random for tests. Seeding does a good job of that, but it's still random. We can work with the number in the loop to find the smallest set that contains all the items required, but this requires tweaking if we ever have to add items. (Not really an issue here with blood types...how often do we add new blood types?) That's my 2 cents on that anyway. |
|
Wel they cant so even 100 is much imo. Cant we say for example that we run x iterations for the amount of possible values with a max of y? In this case we have like 8 options so lets say 24 iterations will do? No idea if times 3 is a valid number but what are the ideas? |
|
@pimjansen Tested locally just now and I've updated the loop counts so they pass locally. I'm assuming this will translate to consistently passing elsewhere, but I'm never 100% certain about randomization and seeding on different machines. |
|
@localheinz Updates made. That will definitely help to keep those tests from growing too far. |
|
Thank you, @anubisthejackle! |
What is the reason for this PR?
To improve the test suite around blood tests so it fails should any blood type, or RH, not be present. Previously it would pass if any blood type and RH were present. Now all must exist.
Author's checklist
Summary of changes
I've updated the Blood Test suite inline with how tests are handled in
en_US/PersonTestregarding randomized data. I don't believe this is a perfect solution, but it does correct the potential issue where breaking changes in the Blood class did not break the tests.Review checklist