metrumresearchgroup / review

helpful tools for organizing and performing quality control (QC) tasks
https://metrumresearchgroup.github.io/review/
2 stars 0 forks source link

clock-dependent dirSummary test failure #58

Closed kyleam closed 6 months ago

kyleam commented 6 months ago

Depending on the local time zone and current time, the below test failure can be triggered:

  ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Failure ('test-dirSummary.R:37:3'): dirSummary captures the expected QC status of all scripts ──
  as.Date(dirSummaryRes$data$`Latest edit`[1]) (`actual`) not equal to Sys.Date() (`expected`).

  `actual`:   "2023-12-05"
  `expected`: "2023-12-04"

That expectation is invalid because Sys.Date() is the current day for the current time zone.

One fix would be to replace the Sys.Date() value with a date for UTC (say format(Sys.time(), "%Y-%m-%d", tz = "UTC")). However, that would leave a harder-to-hit failure: if the test is executed right at the boundary of a day change, with the svnInfo call before and the Sys.time() call for the assertion after, the results would not match.

Instead you could avoid converting to a date. Subtract the POSIXct value in dirSummaryRes from Sys.time(), and assert that the time difference is under N seconds. N should be large enough that, if it were to be reached, it'd be an indication that something was going very wrong with the test run (say 120 seconds in this case?).

cc: @andersone1 @michaelmcd18

andersone1 commented 6 months ago

@kyleam is this blocking this version of review from getting into the next mpn? (I e how urgent is this)

kyleam commented 6 months ago

is this blocking this version of review from getting into the next mpn?

No.

andersone1 commented 6 months ago

is this blocking this version of review from getting into the next mpn?

No.

@kyleam Ok, that's good to know, thanks for bringing this to our attention.

@michaelmcd18 i can review the PR for this issue whenever it's ready

michaelmcd18 commented 6 months ago

Great idea @kyleam, we'll implement this: Instead you could avoid converting to a date. Subtract the POSIXct value in dirSummaryRes from Sys.time(), and assert that the time difference is under N seconds. N should be large enough that, if it were to be reached, it'd be an indication that something was going very wrong with the test run (say 120 seconds in this case?).