Skip to content

test(placement): check long-image placement and share the default report - #1419

Merged
KnifeLemon merged 1 commit into
mainfrom
feat/placement-test-followups
Nov 12, 2026
Merged

KnifeLemon merged 1 commit into
mainfrom
feat/placement-test-followups

Conversation

@SeoJCalive

Copy link
Copy Markdown
Collaborator

What does this change?

Follow-up to your review of #914: the default-placement tests now check the long-image (CombinePages) slots by the route that would actually run, and the full placement report is computed once and shared. Only tests/Filee.App.Tests/DefaultPlacementTests.cs changes; three implementation choices I'd like your opinion on are under Notes for review.

Type of change

  • Bug fix
  • New feature or improvement
  • New format or engine
  • Translation
  • Documentation
  • Build, CI or dependencies

How was it tested?

  • dotnet format --verify-no-changes: dotnet format Filee.slnx --verify-no-changes --no-restore --severity warn exits 0.
  • dotnet test: placement and availability tests 29/29, Core full suite 1,342 passed / 4 skipped / 0 failed on 5b91b379. The App full suite could not finish on my Linux server (see below), so it is left to this PR's CI.
  • Tried it in the app (what you converted or clicked, and on which OS): not applicable; the change is test-only.
Details

Rebased onto 5b91b379 before measuring the new baseline.

  • Release solution build: 26 existing warnings before and after, with identical warning sets and no errors.
  • Using the same command in one fresh process each, the DefaultPlacementTests class runner time was 33 s before and 7 s after (wall: 40.33 s before and 36.70 s after, the latter measured while another worktree was building on the same server; 8 tests before, 10 after). The default full report is computed once instead of five times; the small probe traversal is separate.
  • Placement and availability tests together: 29/29 pass; the shared report factory count is 1. Both static-state tests pass in separate processes.
  • The project-path command dotnet test tests/Filee.App.Tests -c Release --filter FullyQualifiedName~DefaultPlacementTests works with the X11 wrapper and TMPDIR: 10/10 pass.
  • Core full suite on 5b91b379 with this change: 1,342 passed, 4 skipped, 0 failed (the change is test-only in Filee.App.Tests).
  • App full suite on 47e24cd3 with this change: 2,281 passed, 6 failed, 28 skipped (2,315 total; the clean 47e24cd3 run had 2,313 total, the difference being the two new tests). All six failures also fail on the clean 47e24cd3 run in this environment: AnnotateWindowTests.A_label_is_drawn_as_its_box_showed_it_while_it_was_typed (Hindi), RenderTests.Engines_page_shows_versions_descriptions_and_download_speed, both GnomePointerBridgeTests.Real_unix_socket_* tests, and the Japanese cases of LocalizationResourceTests.Han_characters_take_the_embedded_font_of_the_ui_languages_region and FileeFontCollectionTests.A_letter_Noto_Sans_lacks_reads_its_family_and_leaves_later_ones_unread. No failure appears only with this change.
  • On c458122f, the local App full run did not complete: the test host aborted after about 3 h 56 min when this server's per-user thread limit was reached (reported by .NET as out of memory). The failures it recorded before that point were four of the same pre-existing ones (both GNOME socket tests and the two Japanese font cases).
  • Six mutations are detected, and each restoration is rebuilt and passes its named filter. They cover removal of the request, an always-true condition, an image-donut placement, removal of sharing, a graph change during traversal, and removal of the graph assertion. To keep all writes inside the test file, the image-placement mutation adds the slot to the returned default profiles rather than editing product defaults.
  • The graph fingerprint and partial environment match between the before/after classification reports. Two office inputs change classification, pub and the newly added p7m (P7mConverter has a direct edge, so p7m→PNG is reported as ordinary without a PDFium step); no new generic-only slot appears.

Screenshots (UI changes)

None; this change has no UI.

Checklist

  • New UI strings are in all src/Filee.App/Assets/i18n/*.json files
  • New third-party code or binaries are listed in THIRD-PARTY-NOTICES.md
  • The PR does one thing (unrelated changes go in a separate PR)

Notes for review

In the review, you asked for long-image classification in a follow-up: CombinePages can mean an ordinary PNG conversion on a picture rather than the named page operation. You also suggested a static Lazy<Report> as a later, non-blocking way to avoid recalculating the whole report, and said PDF merge did not need another special-operation check. This PR covers those two directions and leaves PDF merge out.

What changed:

  • Add the CombinePages request and selected-route classification without changing product code.
  • Cover real PDF, document, presentation, hwp/hwpx, PNG and pub inputs; use a synthetic catalog to distinguish the PDF route from a cheaper direct PNG route and exclude multipage TIFF.
  • Share the default report with Lazy<Report> and assert both reference identity and one factory invocation.
  • Preserve the graph/environment diagnostics, expand failure diagnostics, and print each default long-image input.
  • Keep the existing usability, generic-only, special-job and known-preset checks.

N-up adds palette presets and a PDF-to-PDF option but no default donut slots; it is outside this special-operation list.

Implementation choices for your opinion:

  1. Selected route in a test-only helper. Would you be comfortable with checking the selected route in this test-only helper, rather than adding a public Preset method? It recognizes a pdfium step from PDF to a target other than TIFF/GIF/APNG/MNG, with the same catalog, priorities and frame/motion/live/OCR/translation flags used for availability. This counts documents that actually render through PDF, while rejecting an available but unselected PDF detour. It does not open files or count pages: a one-page PDF is still a page-capable format here.
  2. Current placements. The current defaults differ from the PDF-only placement mentioned in the review: long image is on the backs of office, presentations and hwp, and not on the PDF donut. Would you agree with retaining those placements and the empty KnownGenericOnly list? All three have runnable inputs that satisfy the operation. pub is the office exception: its selected LibreOffice-to-PNG route bypasses PDFium, so it changes from AsNamed to Generic. It does not make the whole slot GenericOnly. Mail inputs keep their existing Unusable classification.
  3. Sharing. For sharing, would you prefer this small arrangement: initialize through an AvaloniaFact accessor, retain the first traversal's graph check, and use a one-slot probe for the independent graph regression? The probe exercises both an unchanged traversal and a deliberate Priority change, restoring the original reference in finally. It also catches removal of the graph assertion. The shared report is a snapshot of its initial traversal, not a promise that the graph stays unchanged for the assembly's lifetime.

Classify CombinePages using the selected PDFium render step, including
paged document inputs while distinguishing direct picture conversions.
Keep the default report in a Lazy snapshot and exercise its graph guard
with a small normal and deliberately changed traversal.

Cover route selection, report sharing and graph-guard regressions without
changing product code or default placements.

Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>

@KnifeLemon KnifeLemon left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@SeoJCalive 님, 후속 PR 고마워요. #914 리뷰에서 부탁한 두 가지(긴 이미지 분류, 보고서 한 번만 계산)가 그대로 들어왔고, mutation 여섯 개로 실패 경로를 확인해 주셔서 믿고 볼 수 있었어요. CI도 전부 통과했어요.

물어보신 세 가지

  1. 선택된 경로를 테스트 헬퍼에서 확인하는 방식: 이대로 좋아요. 긴 이미지는 PdfiumConverter가 실행 중에 정하는 거라서, Preset에 public 메서드를 만들면 앱에서 아무도 안 쓰는 두 번째 기준이 생겨요. CombinesPagesOf는 PresetAvailability와 같은 플래그로 Plan을 부르고, PdfiumConverter의 조건(pdfium 단계, 여러 페이지 대상 제외)과 똑같이 맞춰져 있어서 실제로 돌 경로를 보고 있어요.
  2. 지금 배치 유지: 네, 유지해 주세요. 제 리뷰의 "PDF 도넛에만 있다"는 말은 #914 머지 다음 날 #806 슬롯 표로 기본 배치가 바뀌면서 맞지 않게 됐어요. 짚어 주셔서 고마워요. 슬롯마다 AsNamed 입력이 있으니 KnownGenericOnly가 비어 있는 게 맞고, pub이 Generic으로 나오는 것도 맞아요(LibreOffice Draw의 PNG 내보내기는 첫 페이지만 나와요). 그 부분은 이 PR에서 다룰 건 아니에요.
  3. 공유 방식: 좋아요. MainWindow 컬렉션이 DisableParallelization = true이고 AvaloniaFact가 헤드리스 디스패처 하나에서 돌아서, Lazy와 Priority 변경이 다른 테스트와 겹치지 않아요. 기본 Lazy는 예외도 캐시하니까 그래프 검사가 실패하면 다섯 테스트에 같은 메시지가 나오는데, 오히려 보기 편해요. 공유 보고서가 첫 순회의 스냅숏이라는 설명도 정확해요.

사소한 것 (선택, 머지에 영향 없음)

  • Graph_fingerprint_is_stable_during_the_run의 Assert.Throws<Xunit.Sdk.TrueException>은 ClassifyAll의 다른 Assert.True(알 수 없는 프리셋)로도 통과할 수 있어요. 지금은 to-png라서 괜찮지만, 반환된 예외 메시지에 "Graph changed during ClassifyAll"이 들어 있는지까지 보면 더 단단해져요.
  • CombinesPagesOf의 ("tiff" or "gif" or "apng" or "mng")는 PdfiumConverter.MultiPageTargets와 같은 목록이에요. 같이 바뀌도록 주석으로 출처를 남겨 두면 좋겠어요.
  • p7m이 "Partly generic"으로 나오는 건 테스트가 p7m → png 직행 간선을 고르기 때문이에요. 앱에서는 봉투를 열고 안쪽 파일 기준으로 경로를 짜서(PlanThrough) 서명된 PDF나 DOCX도 긴 이미지가 돼요. 원하시면 한 줄 주석이면 충분해요.

머지할게요.

@KnifeLemon
KnifeLemon merged commit 3fde9e7 into main Nov 12, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants