Integration tests: mark per-test and consolidate uploads#8068
Merged
Conversation
Two refinements to the WebLibraryIntegration tests:
1. Move [Category("Integration")] from fixture level to the individual
tests that actually hit the network. Most of BookUploadAndDownloadTests
(17 of its tests: url parsing, settings sanitizing, path logic) and
BloomS3ClientTests (AvoidThisFile, GetBaseUrl) are local-only and
fast, and now run even when Integration is excluded: 65 of the
namespace's 72 tests run in 0.6s with
--filter TestCategory!=Integration. BloomS3StandardUpDownloadTests
and ProblemBookUploaderTests stay fixture-marked because every test
in them needs the network.
2. Consolidate uploads, since profiling showed each upload costs two
long-running server operations (mostly waiting on the server):
- UploadBook_SetsInterestingParseFieldsCorrectly merged into
UploadBook_SameId_Replaces: both needed an upload + same-id
re-upload, so one cycle now carries both sets of assertions.
- UploadBook_FillsInMetaData merged with
DownloadUrl_GetsDocument_AndSettings: one book with a missing
instance id AND collection settings serves both.
This cuts the fixture from 7 uploads (14 long-running operations)
to 4 (8), keeping all assertions and all download variants.
(Considered shrinking upload payloads too, but the test books are
already just a handful of tiny text files; the cost is per-operation
server latency, not bytes.)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
| Filename | Overview |
|---|---|
| src/BloomTests/WebLibraryIntegration/BookUploadAndDownloadTests.cs | Fixture-level [Category("Integration")] replaced with per-test attributes; two pairs of tests merged to share upload cycles with all assertions preserved, dest/dest2 separation added to avoid path collisions, and the previously flagged updateSource != "BloomDesktop old" guard restored on the re-upload block. |
| src/BloomTests/WebLibraryIntegration/BloomS3ClientTests.cs | Fixture-level [Category("Integration")] removed; only DownloadBook_DoesNotExist_Throws (the one S3 network call) keeps the attribute; AvoidThisFile and GetBaseUrl tests are now correctly classified as local-only. |
| PAPERCUTS.md | New file documenting 9 known environmental test failures (missing BloomPdfMaker.exe and Test-XMatter packs) in the agent output tree, with format and house rules as specified by the bloom-team-skills papercut convention. |
Reviews (3): Last reviewed commit: "Restore updateSource re-upload assertion..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The merged UploadBook_SameId_ReplacesFilesAndSetsParseFieldsCorrectly
kept the original's check that a re-upload's updateSource is not the
legacy "BloomDesktop old" only for the first upload; the re-upload
block needs it too, since "BloomDesktop old" also passes the retained
StartsWith("BloomDesktop ") check. (Caught by preflight's local review.)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
[Claude/Fable] Consulted Devin on 2026-07-15 up to commit e2ad251 — review clean (no bugs, no flags). |
andrew-polk
marked this pull request as ready for review
July 15, 2026 21:18
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on #8067 (base branch:
speedUpCSharpTests) — refines the integration tests it marked.Per-test
[Category("Integration")]instead of fixture-level. Most ofBookUploadAndDownloadTests(17 tests: URL parsing, settings sanitizing, path logic) andBloomS3ClientTests(12 tests:AvoidThisFile,GetBaseUrl) are local-only and fast; they now run even when Integration is excluded. With--filter TestCategory!=Integration, 65 of the namespace's 72 tests run in ~0.6s.BloomS3StandardUpDownloadTestsandProblemBookUploaderTestsstay fixture-marked (every test needs the network).Fewer uploads, same assertions. Stack sampling showed each upload costs two long-running server operations (mostly waiting on the server), so:
UploadBook_SetsInterestingParseFieldsCorrectlymerged intoUploadBook_SameId_Replaces→UploadBook_SameId_ReplacesFilesAndSetsParseFieldsCorrectly(both needed an upload + same-id re-upload; one cycle now carries both sets of assertions, plus a new record-id-matches-upload assertion).UploadBook_FillsInMetaDatamerged withDownloadUrl_GetsDocument_AndSettings→ one book with a missing instance id and collection settings serves both, feeding all three download variants (direct S3, plain order-URL, forEdit).Fixture goes from 7 uploads (14 long-running operations) to 4 (8). All 72 namespace tests pass against the live services.
(Considered shrinking upload payloads too, but test books are already a handful of tiny text files; the cost is per-operation server latency, not bytes.)
Also adds a
PAPERCUTS.mdentry documenting the 9 environmental full-suite failures agents see (missingBloomPdfMaker.exeandTest-XMatterpacks in the isolated agent output tree).🤖 Generated with Claude Code
Devin review
This change is