Fix/dicomstudio failing tests#184
Merged
Merged
Conversation
Production code grew several enum cases since these tests were written:
CLIWorkshopTab : 6 → 7 (added .listener)
CLIParameterType : 11 → 12 (added .flagPicker)
NavigationDestination : 11 → 16 (added .volumeViewer, .jp3dComparison,
.dicomWeb, .fileOperations,
.integrationTesting)
E2E networking cases : 11 → 14
Integration totalTests: 38 → 41
Also corrects ToolCatalogHelpers.totalToolCount from 33 → 32 to match
the actual count returned by allTools() (the listener tab has no tools
yet).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ogic
Three tests asserted the pre-special-casing buildCommand output. Production
deliberately changed two behaviours that these tests pre-date:
- DIMSE tools (dicom-echo / dicom-query / dicom-send / dicom-retrieve /
dicom-qr / dicom-mwl / dicom-mpps) now collapse host[:port] into a
single positional argument and skip the --host flag form, matching
the dcmtk-style CLI convention.
- Mapped tokens from internal parameters (e.g. --uri from a cliMapping)
are now deferred until after the first positional argument so the
command reads "tool subcommand <url> --flag …" instead of
"tool subcommand --flag <url> …".
Both changes have explanatory comments in CommandBuilderHelpers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QIDOQuery stores parameters keyed by the GGGGEEEE tag UID per the QIDO-RS spec (e.g. "00100010" for PatientName), not by the attribute keyword. Replace the hand-spelled "PatientName" / "PatientID" / "ModalitiesInStudy" / "AccessionNumber" / "StudyDescription" lookups with the typed QIDOQueryAttribute constants so the tests stay in sync if the underlying keying ever changes. Also rewrite the "empty params" test: QIDOQueryParams() defaults limit to 100, so the resulting query is never truly empty — the test now asserts the expected default-limit-only output. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DICOMFile.read(force:) falls back to readLegacyFile, which uses a permissive "looks like DICOM" heuristic that accepts any data whose first two bytes form an even uint16 < 0x7FFF. That matches a lot of plain ASCII (e.g. "This is not a DICOM file" — first bytes T h → 0x6854 — passes), letting parseAllMetadata silently return a result made entirely of UUID() defaults instead of throwing. After the force fallback, require at least one of the four identifying UIDs every real DICOM instance must carry (SOP instance, SOP class, study instance, series instance) and throw parsingFailed otherwise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DICOMwebViewModel and MainViewModel both load persisted JSON from ~/Library/Application Support/DICOMStudio at init, and the failing tests instantiated them with no arguments — so they read whatever profiles and library entries had accumulated from previous test runs (31 phantom server profiles spanning two weeks of leftover state and one phantom study). Both ViewModels already accept dependency-injected storage; introduce makeIsolatedViewModel() / makeIsolatedMainViewModel() helpers that back the storage with a fresh tmp directory per test, and convert the six failing tests to use them. The helpers are reusable for future tests that touch persistent state. Also swap testConnectionUnreachable's "https://pacs.example.com" target for "http://127.0.0.1:1" — port 1 refuses immediately, so the test fails fast instead of waiting on a real-internet timeout (the previous run took 1207 s walking the polluted profile list). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tudy DICOMwebClientFactory.buildQIDOQuery calls QIDOQuery.modality(_:) which stores under tag UID 0008,0060 (series-level Modality), not 0008,0061 (study-level ModalitiesInStudy). The previous fix keyed on the wrong attribute and the modality tests started failing again. Match the test to the production path. Whether buildQIDOQuery should switch on QIDOQueryParams.queryLevel and call .modalitiesInStudy() at study level is a separate question — flagged in a comment for later. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
http://127.0.0.1:1 was the previous attempt — curl refuses it in 12 ms, but URLSession waits the full 300 s resourceTimeout per attempt, and the HTTPClient retries 3 more times → 1208 s total. Switching to a .invalid TLD per RFC 2606 doesn't actually fix the slowness either: NXDOMAIN comes back in 70 ms via curl but URLSession still burns the resourceTimeout × 4 retries, so the test still passes in ~1208 s. .invalid is at least a more truthful URL for "should fail" intent than a real-internet placeholder, and the test no longer risks accidentally hitting whatever pacs.example.com may eventually resolve to. Genuine fix needs either (a) a DICOMwebService mock injected into DICOMwebViewModel.testConnection, or (b) makeClient/makeConfiguration accepting a timeouts: TimeoutConfiguration (= .fast) parameter so tests can dial the per-request timeout down to 10 s. Both are bigger refactors than the triage scope and are left as a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
No description provided.