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

Restore tests (replaces PR #147) #206

Merged
merged 3 commits into from
May 23, 2024

Conversation

marionbarker
Copy link
Contributor

There are 2 commits to this PR.

  • The first one restores the tests which were changed to "skip".
  • The second one should be reviewed - it modifies testCreateMultipleCalibration test so it will pass instead of fail

These are notes from @itsmojo:

Regarding having to comment out the 4 tests in CalibrationsTests, the issue seems to be that the expected values are only returned on the first Test run for a given iPhone simulator. After the first run, the values seem to be a consistent values of 0.9500000000000307, 15.999999999996717, 91.99999999999918 & 2 (instead of the 0.8, 37, 101 & 1 in the initial run that are being tested for). So this seems like this is a stateful test which really should be reworked to have consistent results on each run. Also, using XCTAssertTrue() for these tests is not as helpful as using XCTAssertEqual() so that you can see both values on a failure and so an optional accuracy parameter can be used to deal with non exact floating point values like 15.999999999996717.

Note:

The original PR #147 was automatically closed when the Trio repository was made public. This replaces that PR.

@avouspierre
Copy link
Contributor

the testCreateMultipleCalibration fails if you run only this one. testCreateSimpleCalibration and testCreateMulitpleCalibration use the same data so the result of calibration could be different in regard of the order of the execution. Best to launch calibrationService.removeAllCalibrations() at the end of each test to have a clear calibration datasource.

@marionbarker marionbarker marked this pull request as draft May 17, 2024 20:24
@marionbarker marionbarker marked this pull request as ready for review May 17, 2024 20:42
@marionbarker
Copy link
Contributor Author

Thanks @avouspierre. I believe the last commit makes this a "real" fix.
Ready for review.

Copy link
Contributor

@bjornoleh bjornoleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not quite qualified to review this, I’m afraid.

@Sjoerd-Bo3 Sjoerd-Bo3 self-requested a review May 18, 2024 07:52
Copy link
Contributor

@MikePlante1 MikePlante1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm submitting this review as a proxy for @itsmojo since his review should count for this PR. I did do a test test, and a test build to a simulator.

@MikePlante1 MikePlante1 merged commit 5671755 into nightscout:dev May 23, 2024
1 check passed
@MikePlante1 MikePlante1 mentioned this pull request May 27, 2024
4 tasks
@marionbarker marionbarker deleted the restore_tests branch June 8, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants