Adds tests for DownloadsCounter #3423
Conversation
This change will allow tests to inspect what the persist methods did, and still allows the application to log what happened.
Turbo87
added
A-backend ⚙️
C-internal 🔧
There was a problem hiding this comment.
Looks good to me. One minor comment, feel free to r=me (with or without addressing the comment).
Regarding the duplication of test code, I've never really understood why we put most tests in a separate test crate. I don't think it is necessary, and some of the test infrastructure we've built out in tests/all.rs would be useful for tests in our lib crate. (Plus it is an extra crate that takes time to link. The only advantage that I see is that you can modify these tests without rebuilding the library and bin crates.)
In any case, I'm not worried about this duplication for now. We can look into improving our test infrastructure separately.
| .pending_count | ||
| .fetch_sub(counted_downloads as i64, Ordering::SeqCst); | ||
| let old_pending = self.pending_count.fetch_sub( | ||
| (counted_downloads + discarded_downloads) as i64, |
There was a problem hiding this comment.
Should we print a log message when discarded_downloads is non-zero? I don't think we need to output it for every shard as part of PersistStats, but in the rare case we delete a crate and hit this, it might be nice have a record in the logs.
There was a problem hiding this comment.
Honestly I'm not sure what the value of noticing this in the logs is. Could you provide an example?
|
@bors r=jtgeibel If we decide to log discarded crates I'll do that in a separate PR. |
|
📌 Commit c22ba7c has been approved by |
|
☀️ Test successful - checks-actions |
This fixes #3416 by adding tests for
DownloadsCounter. There are a few notable things in this PR I'd like to highlight.First off, the tests are not implemented in
src/tests, but are implemented in a#[cfg(test)] mod tests {}in the same source file. While this resulted in some code duplication I had to do this because testingpersist_next_shardrequires access to the internals ofDownloadsCounterand I'd prefer not to expose them.This PR also fixes a bug I discovered while writing the test for incrementing a missing version. Inserting a missing version ID in the
version_downloadstable doesn't work thanks to the foreign key, and aborts the wholeINSERTstatement (losing all the downloads in that shard). This could happen in production if a version is deleted between callingincrement()and persisting that shard.To fix that bug I changed the code to do a
SELECT FOR SHAREfrom theversionstable of the version IDs we're about to persist, discarding the versions that are not returned by the query. TheFOR SHAREclause does the equivalent ofRwLock::read, preventing other transactions from updating or deleting the selected rows while allowing otherSELECTs andSELECT FOR SHAREs.There were also some refactorings to make the tests better to write.
This PR is best reviewed commit-by-commit.
r? @jtgeibel