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.

Sign up for GitHub

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

Fix broken playtime entries test. #2094

Merged
merged 3 commits into from
Oct 2, 2024
Merged

Fix broken playtime entries test. #2094

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71ec86e
Fix broken playtime entries test.
dbernstein Oct 1, 2024
75c023d
Test for aggregation for multiple invocations on the same resource wi…
dbernstein Oct 1, 2024
98e9cda
sqlalchemy between is inclusive - it should be exclusive.
dbernstein Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion tests/manager/scripts/test_playtime_entries.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,11 +576,13 @@ def test_do_run(
# We're using the RecursiveEquivalencyCache, so must refresh it.
EquivalentIdentifiersCoverageProvider(db.session).run()

# one month + 3 days ago
playtime(
db.session, identifier, collection, library, dt1m(3), 1, loan_identifier
)
# one month + 4 days ago
playtime(
db.session, identifier, collection, library, dt1m(31), 2, loan_identifier
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@tdilauro tdilauro Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

db.session, identifier, collection, library, dt1m(4), 2, loan_identifier
)
playtime(
db.session, identifier, collection, library, dt1m(-31), 60, loan_identifier
Expand Down