Fix the sweeper monitor commit failures. #2040
Conversation
dbernstein
force-pushed
the
fix-sweep-monitor
branch
from
762ddf5 to
d6310bf
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2040 +/- ##
=======================================
Coverage 90.77% 90.77%
=======================================
Files 342 342
Lines 40513 40511 -2
Branches 8770 8769 -1
=======================================
Hits 36774 36774
+ Misses 2484 2483 -1
+ Partials 1255 1254 -1 ☔ View full report in Codecov by Sentry. |
dbernstein
force-pushed
the
fix-sweep-monitor
branch
2 times, most recently
from
ef9b67e to
2fee790
Compare
| lane = circulation_fixture.db.session.merge(lane) | ||
| session = circulation_fixture.db.session | ||
| lane = session.merge(lane) | ||
| session.expire_all() |
There was a problem hiding this comment.
This update was necessary to get the tests to pass in the ci environment. I was seeing a detached instance failure.
| @@ -465,7 +466,8 @@ def test_get_items_from_query(self, novelist_fixture: NoveListFixture): | |||
|
|
|||
| # Set up a book for this library. | |||
| edition = novelist_fixture.db.edition( | |||
| identifier_type=Identifier.ISBN, publication_date="2012-01-01" | |||
| identifier_type=Identifier.ISBN, | |||
| publication_date=dateutil.parser.parse("2012-01-01"), | |||
There was a problem hiding this comment.
Another side effect of removing that commit(). This one is a bit of a puzzler. I'm not sure how it worked in the first place. But maybe strings are automatically converted to date objects when committed?
| @@ -123,7 +124,10 @@ def test_import( | |||
| assert "Feedbooks" == moby_dick_edition.data_source.name | |||
|
|
|||
| assert "Test Publisher" == moby_dick_edition.publisher | |||
| assert datetime.date(2015, 9, 29) == moby_dick_edition.published | |||
| assert ( | |||
There was a problem hiding this comment.
This change seems to be a result of expiring the licensepool.
| @@ -198,7 +199,10 @@ def test_opds2_importer_correctly_imports_valid_opds2_feed( | |||
| assert data.data_source == moby_dick_edition.data_source | |||
|
|
|||
| assert "Test Publisher" == moby_dick_edition.publisher | |||
| assert datetime.date(2015, 9, 29) == moby_dick_edition.published | |||
| assert ( | |||
| datetime.datetime(2015, 9, 29, tzinfo=tzutc()) | |||
| @@ -255,7 +259,10 @@ def test_opds2_importer_correctly_imports_valid_opds2_feed( | |||
| assert data.data_source == huckleberry_finn_edition.data_source | |||
|
|
|||
| assert "Test Publisher" == huckleberry_finn_edition.publisher | |||
| assert datetime.date(2014, 9, 28) == huckleberry_finn_edition.published | |||
| assert ( | |||
| datetime.datetime(2014, 9, 28, tzinfo=tzutc()) | |||
| # to get LicensePool.delivery_mechanisms to notice that it's | ||
| # out of date. | ||
| if autocommit: | ||
| _db.commit() |
There was a problem hiding this comment.
This commit buried deep in the code is deeply offensive. On the plus side, I appreciate the TODO note.
| @@ -745,6 +744,8 @@ def _update_hold_end_date( | |||
| days=default_reservation_period | |||
| ) | |||
|
|
|||
| _db.expire_all() | |||
There was a problem hiding this comment.
At least one unit test in the test_api.py tests was breaking because the holds collection associated with a patron was stale. This update fixed it. Surprisingly expiring the pool alone did not fix it.
buried in the depths of the code.
dbernstein
force-pushed
the
fix-sweep-monitor
branch
from
0443cff to
e9d8e6e
Compare
Description
The Overdrive Sweeper monitor was failing because buried deep in the code was a
commit()that was breaking the nested transactions used in the sweeper.I removed the
commitand used sqlalchemy'sexpirefacility to ensure that objects were requerying the transactional state when necessary.expiresimply requires that any subsequent access to the objects in the session context are refreshed with the latest known state within the transaction.Motivation and Context
https://ebce-lyrasis.atlassian.net/browse/PP-1690
How Has This Been Tested?
I have manually tested the overdrive format sweep to ensure that it is working.
Checklist