refactor(scraper): Phase 3 class decomposition + fix all 37 pre-existing test failures by jbarden · Pull Request #735 · astar-development/astar-dev-mono

jbarden · 2026-07-11T20:40:03Z

Summary

Phase 3 (class decomposition) of the scraper code review docs/astar-dev-wallpaper-scraper/code-review-2026-07-10.v2.md, plus a follow-up applying the new leave-it-better rule: all 37 pre-existing unit-test failures on main are fixed. One commit per concern, behaviour-preserving throughout — no production code changed by the test fixes.

Refactor (issues #731–#734):

FileClassificationService split — import/export concern moved to a new FileClassificationImportExportService; the now-unused Logger dependency dropped from FileClassificationService.
ImagePageService decomposed — new ImageDownloader (HTTP retrieve + retry) and ImagePersistence (file save, dimension probe, broadcast, FileDetailEntity persistence); dead ScrapeConfiguration/IFileSystem constructor parameters removed (14 → 10); await-then-Match loop replaced with a short-circuiting Result fold.
AppCompositionRoot extracted — all DI registrations and host construction moved out of App.axaml.cs.
ImportScrapeConfigurationAsync — property-by-property copying extracted into one update method per configuration section.

Leave-it-better (new CLAUDE.md rule + fixes):

New Definition-of-Done rule: pre-existing test failures must be fixed in the same branch; production behaviour is the specification; tests for deleted features get deleted.
16 scraper test failures fixed (stale expectations vs. current production: CombinePath rooted-segment rejection, tag-category filtering, ScrapedTag.Category string type, NOCASE category reuse, seeded Colour root).
21 OneDrive Sync Client test failures fixed (CancellationToken stub/assertion mismatches vs. production's default tokens, IncludeInSearch filter in GetAllCategoriesAsync, ScrapedTags → FileClassifications migration leftovers).

Type of change

Related issues / links

Closes #731, Closes #732, Closes #733, Closes #734

How was this tested?

Unit tests added/updated
Manual validation
Not applicable

Import/export tests moved to GivenAFileClassificationImportExportService; new GivenAnImageDownloader and GivenAnImagePersistence test classes; 37 stale tests realigned with production behaviour; GivenAScrapedTagEntity reworked as GivenAPersistedFileClassificationCategory.

Impacted areas

apps/desktop/Scraper, apps/desktop/AStar.Dev.OneDrive.Sync.Client.Tests.Unit (tests only), CLAUDE.md

TDD Checklist (required)

dotnet clean && dotnet build --no-restore: 0 errors; remaining warnings are pre-existing on main (5× Avalonia AVLN3001 for DI-only view constructors, 3× NU1903 SQLitePCLRaw advisory in spikes/).

dotnet test --no-build, full solution: Failed: 0 across all 13 test projects — Scraper 441/441, OneDrive Sync Client unit 2156/2156, integration 27/27. Main currently has 37 failures (16 scraper + 21 OneDrive); this branch takes the suite fully green.

Public API changes

FileClassificationService constructor loses Logger; ExportClassificationsAsync/ImportClassificationsAsync moved to FileClassificationImportExportService (both internal).
ImagePageService constructor: 14 → 10 parameters (ImageDownloader + ImagePersistence in; ScrapeConfiguration, IFileSystem, IImageRetriever, IImageSaver, ImageBroadcaster, IImageDimensionReader out).
ClassificationsView now takes FileClassificationImportExportService.

How to run tests locally

# Restore and run tests
dotnet restore AStar.Dev.slnx
dotnet test --verbosity normal

Notes for reviewers

Ensure CI passed for all OS runners where configured.
Every test fix treats production as correct; none of the 37 failures exposed a production bug. Notables: category Name intentionally has no unique index (same name under different parents); ScrapedTagExtensions.ToDomain still hardcodes ParentId = 1 (pre-existing TODO in production, unchanged here).
Review recommended a further FileClassificationRepository extraction (EF tracking out of FileClassificationService.ClassifyAsync); deferred because EnsureTracked/FindOrCreate* are bound to the classify unit-of-work's DbContext lifetime — splitting them without redesigning that pipeline would add indirection, not clarity.

🤖 Generated with Claude Code

… FileClassificationService Import/export was a separate domain concern living inside the classification service. Move it, its upsert/fallback helpers, and its tests into a dedicated service; drop the now-unused Logger dependency from FileClassificationService. Closes #731 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…tence, and orchestrator ImagePageService mixed orchestration with HTTP retry, file I/O, dimension probing, DB persistence, and broadcasting behind 14 constructor parameters (two of them dead). Extract ImageDownloader (retrieve + retry) and ImagePersistence (save, dimensions, broadcast, FileDetailEntity persistence); drop the dead ScrapeConfiguration and IFileSystem parameters; replace the await-then-Match loop in GetTheImagePagesAsync with a Result fold. Closes #732 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ositionRoot App mixed Avalonia framework glue with the entire composition root. Move host/builder creation and all service registrations into a dedicated AppCompositionRoot; App now only initialises the database, configures the desktop lifetime, and surfaces configuration errors. Closes #733 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…peConfigurationAsync 56 lines of property-by-property copying now reads as one step per configuration section: connection strings, user configuration, search configuration, search categories, scrape directories. Closes #734 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

github-actions · 2026-07-11T20:42:41Z

Test results

3 169 tests +2 189 3 168 ✅ +2 205 2m 23s ⏱️ + 2m 0s
13 suites + 2 1 💤 ± 0
13 files + 2 0 ❌ - 16

Results for commit da074e5. ± Comparison against base commit c326573.

This pull request removes 11 and adds 2200 tests. Note that renamed tests count towards both.

AStar.Dev.Wallpaper.Scraper.Tests.Unit.Services.GivenAFileClassificationService ‑ when_classifying_with_a_new_tag_and_the_unclassified_root_already_exists_then_it_is_reused
AStar.Dev.Wallpaper.Scraper.Tests.Unit.Services.GivenAFileClassificationService ‑ when_classifying_with_a_tag_matching_an_existing_category_with_different_casing_then_no_duplicate_category_is_created
AStar.Dev.Wallpaper.Scraper.Tests.Unit.Services.GivenAFileClassificationService ‑ when_classifying_with_a_tag_matching_no_existing_category_then_a_level_two_category_under_the_unclassified_root_is_created
AStar.Dev.Wallpaper.Scraper.Tests.Unit.Services.GivenAFileClassificationService ‑ when_classifying_with_a_tag_matching_no_existing_category_then_the_junction_row_links_to_the_newly_created_category
AStar.Dev.Wallpaper.Scraper.Tests.Unit.Services.GivenAFileClassificationService ‑ when_classifying_with_two_new_tags_then_both_share_a_single_unclassified_root
AStar.Dev.Wallpaper.Scraper.Tests.Unit.Services.GivenAFileClassificationService ‑ when_exporting_classifications_then_categories_and_keywords_are_returned
AStar.Dev.Wallpaper.Scraper.Tests.Unit.Services.GivenAFileClassificationService ‑ when_importing_a_classification_whose_keyword_already_exists_with_different_casing_then_no_duplicate_keyword_is_added
AStar.Dev.Wallpaper.Scraper.Tests.Unit.Services.GivenAFileClassificationService ‑ when_importing_a_classification_whose_primary_upsert_fails_then_it_is_reparented_under_unclassified
AStar.Dev.Wallpaper.Scraper.Tests.Unit.Services.GivenAFileClassificationService ‑ when_importing_a_new_classification_then_it_is_added
AStar.Dev.Wallpaper.Scraper.Tests.Unit.Support.GivenTheTagRules ‑ when_a_tag_has_a_null_category_then_it_is_excluded_from_rule_processing_but_its_text_is_kept_in_tags
…

AStar.Dev.OneDrive.Sync.Client.Tests.Integration.Data.GivenSyncedItemRepository_WhenSearchingForDuplicates ‑ when_duplicates_exist_for_one_account_they_do_not_appear_in_another_accounts_results
AStar.Dev.OneDrive.Sync.Client.Tests.Integration.Data.GivenSyncedItemRepository_WhenSearchingForDuplicates ‑ when_files_have_same_name_but_different_sizes_then_not_returned
AStar.Dev.OneDrive.Sync.Client.Tests.Integration.Data.GivenSyncedItemRepository_WhenSearchingForDuplicates ‑ when_files_have_same_size_but_different_names_then_not_returned
AStar.Dev.OneDrive.Sync.Client.Tests.Integration.Data.GivenSyncedItemRepository_WhenSearchingForDuplicates ‑ when_files_share_size_and_filename_then_both_are_returned
AStar.Dev.OneDrive.Sync.Client.Tests.Integration.Data.GivenSyncedItemRepository_WhenSearchingForDuplicates ‑ when_folders_share_size_and_name_they_are_excluded_from_duplicate_detection
AStar.Dev.OneDrive.Sync.Client.Tests.Integration.Data.GivenSyncedItemRepository_WhenSearchingForDuplicates ‑ when_items_have_null_size_they_are_excluded_from_duplicate_detection
AStar.Dev.OneDrive.Sync.Client.Tests.Integration.Data.GivenSyncedItemRepository_WhenSearchingForDuplicates ‑ when_no_duplicate_files_exist_then_returns_empty_list
AStar.Dev.OneDrive.Sync.Client.Tests.Integration.Data.GivenSyncedItemRepository_WhenSearchingForDuplicates ‑ when_three_files_share_size_and_name_then_all_three_are_returned
AStar.Dev.OneDrive.Sync.Client.Tests.Integration.Data.GivenTheMigrationDropFileClassificationRulesTable ‑ when_all_migrations_are_applied_then_file_classification_rules_table_does_not_exist
AStar.Dev.OneDrive.Sync.Client.Tests.Integration.FileClassification.GivenFileAutoCategorisor ‑ when_path_contains_documents_token_then_result_is_none
…

♻️ This comment has been updated with latest results.

…ixed, production is the specification Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…t production behaviour Production is the specification for all of these: - GivenADatabaseResetRepository: CombinePath rejects rooted segments; seed a RootDirectory and a relative BaseSaveDirectory. - GivenTheTagRules: tags without a category are excluded from the outcome's Tags, not kept. - GivenAnImportExportService: ScrapedTag.Category is a string; the test JSON still used the old numeric category id. - GivenAFileClassificationService: classify reuses the NOCASE-matched existing category (highest level wins); the create-under-Unclassified fallback is unreachable from tag matches, and AppDbContext seeds a 'Colour' root that whole-table counts must account for. Scraper suite: Failed: 0, Passed: 441. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…current production behaviour Production is the specification for all of these: - GivenASettingsViewModel / GivenAStartupService / GivenASyncWorker: production calls SaveAsync/SetCultureAsync/GetCachedAccountIdsAsync with the default CancellationToken; stubs and Received assertions pinned to TestContext.Current.CancellationToken never matched. Use Arg.Any<CancellationToken>(). - GivenAFileClassificationRepository: GetAllCategoriesAsync filters on IncludeInSearch; seed rows now opt in. - ScrapedTags was replaced by FileClassifications (#717): the DbSet existence test now asserts absence, and GivenAScrapedTagEntity is reworked as GivenAPersistedFileClassificationCategory — duplicate names are allowed by design (no unique index), and retrieval must not collide with the seeded 'Colour' root. OneDrive suite: Failed: 0, Passed: 2156. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

jbarden and others added 4 commits July 11, 2026 21:27

jbarden requested a review from a team July 11, 2026 20:40

astar-development-jb enabled auto-merge (squash) July 11, 2026 20:43

jbarden and others added 3 commits July 11, 2026 21:51

docs: add leave-it-better rule — pre-existing test failures must be f…

6b9c2e2

…ixed, production is the specification Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

jbarden changed the title ~~refactor(scraper): Phase 3 — class decomposition (FileClassificationService, ImagePageService, App composition root, config import)~~ refactor(scraper): Phase 3 class decomposition + fix all 37 pre-existing test failures Jul 11, 2026

astar-development-jb approved these changes Jul 11, 2026

View reviewed changes

astar-development-jb merged commit 83beb20 into main Jul 11, 2026
7 checks passed

astar-development-jb deleted the refactor/scraper-phase-3-decomposition branch July 11, 2026 21:11

jbarden mentioned this pull request Jul 12, 2026

refactor(scraper): Phase 4 — cross-cutting CancellationToken propagation #737

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(scraper): Phase 3 class decomposition + fix all 37 pre-existing test failures#735

refactor(scraper): Phase 3 class decomposition + fix all 37 pre-existing test failures#735
astar-development-jb merged 7 commits into
mainfrom
refactor/scraper-phase-3-decomposition

jbarden commented Jul 11, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jbarden commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of change

Related issues / links

How was this tested?

Impacted areas

TDD Checklist (required)

Public API changes

How to run tests locally

Notes for reviewers

Uh oh!

github-actions Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test results

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jbarden commented Jul 11, 2026 •

edited

Loading

github-actions Bot commented Jul 11, 2026 •

edited

Loading