Fix broken playtime entries test. #2094
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2094 +/- ##
=======================================
Coverage 90.65% 90.65%
=======================================
Files 344 344
Lines 40580 40581 +1
Branches 8822 8822
=======================================
+ Hits 36789 36790 +1
Misses 2484 2484
Partials 1307 1307 ☔ View full report in Codecov by Sentry. |
| playtime( | ||
| db.session, identifier, collection, library, dt1m(31), 2, loan_identifier |
There was a problem hiding this comment.
I think this entry is meant to be near the beginning of the month during which the report is being run and, thus, should not be included in the report. dt1m is calculating an offset from the first day of the previous month.
I believe that:
- the
dt1m(31)entry is meant to represent "out of range - after reporting period", and - the `dt1m(-31) entry, "out of range - before reporting period" (it is helpfully labeled to that effect)
There was a problem hiding this comment.
Right I think that test wants to verify that two identifiers on the same library, collection and loan sum the have their seconds summed together. When it was dt1m(31), they were both being summed up to 3 (1+2) because dt1m(31) was in the past. However today is October 1st. dt1m(31) is Sept 1st + 31 days = October 2 in the future.
So the expected values were wrong, but it was also my intent to verify that two invocations with the same parameters at different but value times are summed. I'll update the test.
There was a problem hiding this comment.
But the defaults for that script are to run the previous month, as indicated by the cutoff and until dates in the test; so no entry from the current month should be counted. The until date is after what should be included in the report.
cutoff = date1m(0).replace(day=1)
until = utc_now().date().replace(day=1)Here's the explanation of the until date from the scripts argparser:
help="'Until' date for report in ISO 8601 'yyyy-mm-dd' format."
" The report will represent entries from the 'start' date up until,"
" but not including, this date.",There was a problem hiding this comment.
Okay PR updates. It turns out sqlalchemy's between function is inclusive which is not what we want to align with until definition in the argparser.
I verified that one month back plus 30 days (which is Oct 1st) failed before the change to the where clause. Then it passed with the updates. Then I changed it to 31 so that it will succeed in a 31 day month like October.
…thint a reporting period AND non-counting of invocations in the future.
dbernstein
force-pushed
the
fix-broken-playtime-entries-test
branch
from
e3ee129
to
aec4fed
Compare
| @@ -238,7 +238,12 @@ def _fetch_report_records(self, start: datetime, until: datetime) -> Query: | |||
| sql_max(coalesce(PlaytimeSummary.title, "")).label("title2"), | |||
| count(distinct(PlaytimeSummary.loan_identifier)).label("loan_count"), | |||
| ) | |||
| .where(PlaytimeSummary.timestamp.between(start, until)) | |||
| .where( | |||
| and_( | |||
There was a problem hiding this comment.
This where clause is exclusive whereas between is inclusive.
There was a problem hiding this comment.
Good freakin catch. I don't know how you do it.
There was a problem hiding this comment.
😊 Thanks!
The tests really caught it. I just noticed that we were needing to relax a test that should've passed.
| db.session, identifier, collection, library, dt1m(31), 2, loan_identifier | ||
| db.session, identifier, collection, library, dt1m(4), 2, loan_identifier | ||
| ) | ||
| # one month + 31 days ago : out of range of reporting period |
There was a problem hiding this comment.
Minor quibble about this comment: it sounds like it's over two months ago. Could we re-word to something like:
# out of range: after end of default reporting period
But, good to have this labeled, so that we know what's up next time! For future us! 🥂
| @@ -238,7 +238,12 @@ def _fetch_report_records(self, start: datetime, until: datetime) -> Query: | |||
| sql_max(coalesce(PlaytimeSummary.title, "")).label("title2"), | |||
| count(distinct(PlaytimeSummary.loan_identifier)).label("loan_count"), | |||
| ) | |||
| .where(PlaytimeSummary.timestamp.between(start, until)) | |||
| .where( | |||
| and_( | |||
There was a problem hiding this comment.
😊 Thanks!
The tests really caught it. I just noticed that we were needing to relax a test that should've passed.
dbernstein
force-pushed
the
fix-broken-playtime-entries-test
branch
from
aec4fed
to
98e9cda
Compare
Description
This update fixes a broken test. The test started failing due to a date addition issue that was interacting badly with the current month. I changed it to ensure that the playtime entry in question is always in scope.
Motivation and Context
Build was broken.
How Has This Been Tested?
Checklist