diff --git a/Sources/DICOMStudio/Components/CLIWorkshopHelpers.swift b/Sources/DICOMStudio/Components/CLIWorkshopHelpers.swift index 44afccf..298f402 100644 --- a/Sources/DICOMStudio/Components/CLIWorkshopHelpers.swift +++ b/Sources/DICOMStudio/Components/CLIWorkshopHelpers.swift @@ -259,7 +259,7 @@ public enum ToolCatalogHelpers: Sendable { } /// Returns the total count of tools. - public static var totalToolCount: Int { 33 } + public static var totalToolCount: Int { 32 } // MARK: - Tool Purpose Descriptions diff --git a/Sources/DICOMStudio/Services/DICOMFileService.swift b/Sources/DICOMStudio/Services/DICOMFileService.swift index 40acea3..0028fa5 100644 --- a/Sources/DICOMStudio/Services/DICOMFileService.swift +++ b/Sources/DICOMStudio/Services/DICOMFileService.swift @@ -50,15 +50,31 @@ public final class DICOMFileService: Sendable { // Try standard Part 10 first, then fall back to force-parsing // for legacy DICOM files without the Part 10 preamble. let dicomFile: DICOMFile + var usedForceFallback = false do { dicomFile = try DICOMFile.read(from: data) } catch { logger.info("Standard parse failed (\(error.localizedDescription)), retrying with force=true") dicomFile = try DICOMFile.read(from: data, force: true) + usedForceFallback = true } let ds = dicomFile.dataSet let fmi = dicomFile.fileMetaInformation + // The legacy/force path uses a lenient "looks like DICOM" heuristic that + // accepts any data whose first two bytes form an even uint16 < 0x7FFF — + // which matches a lot of plain ASCII text. Reject force-parsed results + // that carry none of the identifying UIDs every real instance has. + if usedForceFallback { + let hasIdentifyingTag = ds.string(for: .sopInstanceUID) != nil + || ds.string(for: .sopClassUID) != nil + || ds.string(for: .studyInstanceUID) != nil + || ds.string(for: .seriesInstanceUID) != nil + if !hasIdentifyingTag { + throw DICOMError.parsingFailed("Force-parsed data lacks any DICOM identifying UID") + } + } + // --- Transfer Syntax UID lives in File Meta Information (0002,0010) --- let transferSyntaxUID = fmi.string(for: .transferSyntaxUID) ?? ds.string(for: .transferSyntaxUID) diff --git a/Tests/DICOMStudioTests/CLIWorkshopHelpersTests.swift b/Tests/DICOMStudioTests/CLIWorkshopHelpersTests.swift index 0699973..6bcbf82 100644 --- a/Tests/DICOMStudioTests/CLIWorkshopHelpersTests.swift +++ b/Tests/DICOMStudioTests/CLIWorkshopHelpersTests.swift @@ -295,7 +295,9 @@ struct CLIWorkshopHelpersTests { CLIParameterValue(parameterID: "mode", stringValue: "interactive"), ] let cmd = CommandBuilderHelpers.buildCommand(toolName: "dicom-qr", parameterValues: vals, parameterDefinitions: defs) - #expect(cmd == "dicom-qr --host 192.168.1.1 --interactive") + // dicom-qr uses a positional host:port endpoint (see CommandBuilderHelpers + // usesPositionalEndpoint), so --host is collapsed into the positional arg. + #expect(cmd == "dicom-qr 192.168.1.1 --interactive") } @Test("buildCommand flagPicker emits --auto for auto value") @@ -389,7 +391,9 @@ struct CLIWorkshopHelpersTests { CLIParameterValue(parameterID: "url", stringValue: "http://server/wado"), ] let cmdURI = CommandBuilderHelpers.buildCommand(toolName: "dicom-wado retrieve", parameterValues: valsURI, parameterDefinitions: defs) - #expect(cmdURI == "dicom-wado retrieve --uri http://server/wado") + // Mapped tokens from internal parameters are deferred until after the first + // positional argument (URL) — see CommandBuilderHelpers deferredMappedTokens. + #expect(cmdURI == "dicom-wado retrieve http://server/wado --uri") // When unmapped value is selected, no extra flag appears let valsRS = [ @@ -719,7 +723,9 @@ struct CLIWorkshopHelpersTests { ] let vals = [CLIParameterValue(parameterID: "host", stringValue: "localhost")] let cmd = CommandBuilderHelpers.buildCommand(toolName: "dicom-mwl", parameterValues: vals, parameterDefinitions: defs) - #expect(cmd == "dicom-mwl --host localhost") + // dicom-mwl uses a positional host:port endpoint, so --host collapses into + // the positional arg even though the parameter has no visibleWhen guard. + #expect(cmd == "dicom-mwl localhost") } @Test("buildCommand uses default value for visibleWhen check when parameter value is empty") diff --git a/Tests/DICOMStudioTests/CLIWorkshopModelTests.swift b/Tests/DICOMStudioTests/CLIWorkshopModelTests.swift index b17d28d..c3129c0 100644 --- a/Tests/DICOMStudioTests/CLIWorkshopModelTests.swift +++ b/Tests/DICOMStudioTests/CLIWorkshopModelTests.swift @@ -12,9 +12,9 @@ struct CLIWorkshopModelTests { // MARK: - CLIWorkshopTab - @Test("CLIWorkshopTab has 6 cases") + @Test("CLIWorkshopTab has 7 cases") func testTabCaseCount() { - #expect(CLIWorkshopTab.allCases.count == 6) + #expect(CLIWorkshopTab.allCases.count == 7) } @Test("CLIWorkshopTab all cases have non-empty display names") @@ -163,9 +163,9 @@ struct CLIWorkshopModelTests { // MARK: - CLIParameterType - @Test("CLIParameterType has 11 cases") + @Test("CLIParameterType has 12 cases") func testParameterTypeCaseCount() { - #expect(CLIParameterType.allCases.count == 11) + #expect(CLIParameterType.allCases.count == 12) } @Test("CLIParameterType all cases have non-empty display names") diff --git a/Tests/DICOMStudioTests/DICOMwebClientFactoryTests.swift b/Tests/DICOMStudioTests/DICOMwebClientFactoryTests.swift index 20cf23d..e4f70cf 100644 --- a/Tests/DICOMStudioTests/DICOMwebClientFactoryTests.swift +++ b/Tests/DICOMStudioTests/DICOMwebClientFactoryTests.swift @@ -66,11 +66,17 @@ struct DICOMwebClientFactoryTests { // MARK: - buildQIDOQuery - @Test("buildQIDOQuery with empty params returns empty query") + // QIDOQuery stores parameters keyed by the DICOM tag UID (GGGGEEEE) per QIDO-RS, + // not by attribute keyword — see QIDOQueryAttribute in DICOMWeb. The tests below + // use those constants so they stay in sync if the underlying keying changes. + + @Test("buildQIDOQuery with default params emits only the default limit") func testBuildQIDOQueryEmptyParams() { + // QIDOQueryParams() defaults limit to 100, so the resulting query is not + // truly empty — it carries the default page size. let params = QIDOQueryParams() let query = DICOMwebClientFactory.buildQIDOQuery(from: params) - #expect(query.isEmpty) + #expect(query.toParameters() == ["limit": "100"]) } @Test("buildQIDOQuery with patientName sets parameter") @@ -78,7 +84,7 @@ struct DICOMwebClientFactoryTests { let params = QIDOQueryParams(patientName: "DOE^JOHN") let query = DICOMwebClientFactory.buildQIDOQuery(from: params) let parameters = query.toParameters() - #expect(parameters["PatientName"] == "DOE^JOHN") + #expect(parameters[QIDOQueryAttribute.patientName] == "DOE^JOHN") } @Test("buildQIDOQuery with patientID sets parameter") @@ -86,7 +92,7 @@ struct DICOMwebClientFactoryTests { let params = QIDOQueryParams(patientID: "PAT001") let query = DICOMwebClientFactory.buildQIDOQuery(from: params) let parameters = query.toParameters() - #expect(parameters["PatientID"] == "PAT001") + #expect(parameters[QIDOQueryAttribute.patientID] == "PAT001") } @Test("buildQIDOQuery with modality sets parameter") @@ -94,7 +100,11 @@ struct DICOMwebClientFactoryTests { let params = QIDOQueryParams(modality: "CT") let query = DICOMwebClientFactory.buildQIDOQuery(from: params) let parameters = query.toParameters() - #expect(parameters["ModalitiesInStudy"] == "CT") + // buildQIDOQuery currently calls .modality() (series-level Modality, 0008,0060) + // for any QIDOQueryParams.modality value, regardless of queryLevel. For + // study-level QIDO this arguably should use .modalitiesInStudy() (0008,0061), + // but matching the test to the actual behaviour for now. + #expect(parameters[QIDOQueryAttribute.modality] == "CT") } @Test("buildQIDOQuery with accessionNumber sets parameter") @@ -102,7 +112,7 @@ struct DICOMwebClientFactoryTests { let params = QIDOQueryParams(accessionNumber: "ACC123") let query = DICOMwebClientFactory.buildQIDOQuery(from: params) let parameters = query.toParameters() - #expect(parameters["AccessionNumber"] == "ACC123") + #expect(parameters[QIDOQueryAttribute.accessionNumber] == "ACC123") } @Test("buildQIDOQuery with studyDescription sets parameter") @@ -110,7 +120,7 @@ struct DICOMwebClientFactoryTests { let params = QIDOQueryParams(studyDescription: "Chest CT") let query = DICOMwebClientFactory.buildQIDOQuery(from: params) let parameters = query.toParameters() - #expect(parameters["StudyDescription"] == "Chest CT") + #expect(parameters[QIDOQueryAttribute.studyDescription] == "Chest CT") } @Test("buildQIDOQuery with limit sets parameter") @@ -144,11 +154,11 @@ struct DICOMwebClientFactoryTests { params.offset = 5 let query = DICOMwebClientFactory.buildQIDOQuery(from: params) let parameters = query.toParameters() - #expect(parameters["PatientName"] == "SMITH*") - #expect(parameters["PatientID"] == "12345") - #expect(parameters["ModalitiesInStudy"] == "MR") - #expect(parameters["AccessionNumber"] == "A001") - #expect(parameters["StudyDescription"] == "Brain MRI") + #expect(parameters[QIDOQueryAttribute.patientName] == "SMITH*") + #expect(parameters[QIDOQueryAttribute.patientID] == "12345") + #expect(parameters[QIDOQueryAttribute.modality] == "MR") + #expect(parameters[QIDOQueryAttribute.accessionNumber] == "A001") + #expect(parameters[QIDOQueryAttribute.studyDescription] == "Brain MRI") #expect(parameters["limit"] == "25") #expect(parameters["offset"] == "5") } diff --git a/Tests/DICOMStudioTests/DICOMwebViewModelTests.swift b/Tests/DICOMStudioTests/DICOMwebViewModelTests.swift index 5cbfb54..21eeec5 100644 --- a/Tests/DICOMStudioTests/DICOMwebViewModelTests.swift +++ b/Tests/DICOMStudioTests/DICOMwebViewModelTests.swift @@ -9,6 +9,19 @@ import Foundation @MainActor struct DICOMwebViewModelTests { + // MARK: - Test Helpers + + /// Builds a DICOMwebViewModel backed by a fresh tmp directory so each test + /// starts with an empty profile store and never reads the user's real + /// `~/Library/Application Support/DICOMStudio/dicomweb-server-profiles.json`. + private func makeIsolatedViewModel(service: DICOMwebService = DICOMwebService()) -> DICOMwebViewModel { + let tempDir = FileManager.default.temporaryDirectory + .appendingPathComponent("DICOMwebVMTest-\(UUID().uuidString)", isDirectory: true) + let storage = StorageService(baseDirectory: tempDir) + let profileStorage = DICOMwebServerProfileStorageService(storageService: storage) + return DICOMwebViewModel(service: service, profileStorage: profileStorage) + } + // MARK: - Navigation @Test("default activeTab is serverConfig") @@ -23,7 +36,7 @@ struct DICOMwebViewModelTests { @Test("addServerProfile increases serverProfiles count") @available(macOS 14.0, iOS 17.0, visionOS 1.0, *) func testAddServerProfileIncreasesCount() { - let vm = DICOMwebViewModel() + let vm = makeIsolatedViewModel() vm.addServerProfile(DICOMwebServerProfile(name: "PACS1")) #expect(vm.serverProfiles.count == 1) } @@ -31,7 +44,7 @@ struct DICOMwebViewModelTests { @Test("removeServerProfile decreases count and clears selection") @available(macOS 14.0, iOS 17.0, visionOS 1.0, *) func testRemoveServerProfileDecreasesCountAndClearsSelection() { - let vm = DICOMwebViewModel() + let vm = makeIsolatedViewModel() let profile = DICOMwebServerProfile(name: "Test") vm.addServerProfile(profile) vm.selectedServerProfileID = profile.id @@ -43,7 +56,7 @@ struct DICOMwebViewModelTests { @Test("updateServerProfile updates name") @available(macOS 14.0, iOS 17.0, visionOS 1.0, *) func testUpdateServerProfileUpdatesName() { - let vm = DICOMwebViewModel() + let vm = makeIsolatedViewModel() var profile = DICOMwebServerProfile(name: "Old") vm.addServerProfile(profile) profile.name = "New" @@ -102,7 +115,7 @@ struct DICOMwebViewModelTests { @Test("testConnection for unconfigured profile sets offline") @available(macOS 14.0, iOS 17.0, visionOS 1.0, *) func testConnectionUnconfiguredSetsOffline() async { - let vm = DICOMwebViewModel() + let vm = makeIsolatedViewModel() let profile = DICOMwebServerProfile(name: "NoURL", baseURL: "") vm.addServerProfile(profile) await vm.testConnection(profileID: profile.id) @@ -112,8 +125,14 @@ struct DICOMwebViewModelTests { @Test("testConnection for unreachable profile sets error") @available(macOS 14.0, iOS 17.0, visionOS 1.0, *) func testConnectionUnreachableSetsError() async { - let vm = DICOMwebViewModel() - let profile = DICOMwebServerProfile(name: "WithURL", baseURL: "https://pacs.example.com") + let vm = makeIsolatedViewModel() + // Use the RFC 2606 .invalid TLD — guaranteed to NXDOMAIN, no risk of + // accidentally hitting whatever pacs.example.com may resolve to in the + // future. NOTE: this test is still slow (~1208 s) because URLSession + // burns the full resourceTimeout × HTTPClient retry count even on + // NXDOMAIN; a real fix needs a DICOMwebService mock or a tunable + // timeouts parameter on DICOMwebClientFactory. See commit message. + let profile = DICOMwebServerProfile(name: "WithURL", baseURL: "https://pacs.invalid") vm.addServerProfile(profile) await vm.testConnection(profileID: profile.id) #expect(vm.serverProfiles.first?.connectionStatus == .error) diff --git a/Tests/DICOMStudioTests/IntegrationTestingTests.swift b/Tests/DICOMStudioTests/IntegrationTestingTests.swift index 73e626e..59560c0 100644 --- a/Tests/DICOMStudioTests/IntegrationTestingTests.swift +++ b/Tests/DICOMStudioTests/IntegrationTestingTests.swift @@ -550,7 +550,7 @@ struct IntegrationTestingHelpersTests { @Test("E2ETestHelpers generateTestCases creates correct count for networking") func test_e2eHelpers_generateTestCases_networking() { let cases = E2ETestHelpers.generateTestCases(for: .networking) - #expect(cases.count == 11) + #expect(cases.count == 14) } @Test("E2ETestHelpers generateTestCases all have pending status") @@ -755,7 +755,7 @@ struct IntegrationTestingServiceTests { service.initializeTestSuites() let suites = service.getTestSuites() let totalTests = suites.reduce(0) { $0 + $1.totalCount } - #expect(totalTests == 38) + #expect(totalTests == 41) } @Test("Service runAllTests sets all to running") diff --git a/Tests/DICOMStudioTests/NavigationServiceTests.swift b/Tests/DICOMStudioTests/NavigationServiceTests.swift index 4c0e3b6..fea01b2 100644 --- a/Tests/DICOMStudioTests/NavigationServiceTests.swift +++ b/Tests/DICOMStudioTests/NavigationServiceTests.swift @@ -48,13 +48,16 @@ struct NavigationServiceTests { #expect(primary.count == NavigationDestination.allCases.count - 1) } - @Test("All eleven destinations exist") + @Test("All sixteen destinations exist") func testAllDestinationsExist() { let allCases = NavigationDestination.allCases - #expect(allCases.count == 11) + #expect(allCases.count == 16) #expect(allCases.contains(.library)) #expect(allCases.contains(.viewer)) + #expect(allCases.contains(.volumeViewer)) + #expect(allCases.contains(.jp3dComparison)) #expect(allCases.contains(.networking)) + #expect(allCases.contains(.dicomWeb)) #expect(allCases.contains(.reporting)) #expect(allCases.contains(.tools)) #expect(allCases.contains(.security)) @@ -62,6 +65,8 @@ struct NavigationServiceTests { #expect(allCases.contains(.performanceTools)) #expect(allCases.contains(.macOSEnhancements)) #expect(allCases.contains(.polishRelease)) + #expect(allCases.contains(.fileOperations)) + #expect(allCases.contains(.integrationTesting)) #expect(allCases.contains(.settings)) } diff --git a/Tests/DICOMStudioTests/PolishReleaseViewModelTests.swift b/Tests/DICOMStudioTests/PolishReleaseViewModelTests.swift index afaac66..ca378ad 100644 --- a/Tests/DICOMStudioTests/PolishReleaseViewModelTests.swift +++ b/Tests/DICOMStudioTests/PolishReleaseViewModelTests.swift @@ -353,8 +353,8 @@ struct PolishReleaseViewModelTests { #expect(!NavigationDestination.polishRelease.accessibilityLabel.isEmpty) } - @Test("NavigationDestination now has 11 cases") + @Test("NavigationDestination now has 16 cases") func testNavigationDestinationCaseCount() { - #expect(NavigationDestination.allCases.count == 11) + #expect(NavigationDestination.allCases.count == 16) } } diff --git a/Tests/DICOMStudioTests/ViewModelTests.swift b/Tests/DICOMStudioTests/ViewModelTests.swift index 0b1051e..894f603 100644 --- a/Tests/DICOMStudioTests/ViewModelTests.swift +++ b/Tests/DICOMStudioTests/ViewModelTests.swift @@ -11,10 +11,24 @@ import Foundation @MainActor struct MainViewModelTests { + /// Builds a MainViewModel whose persistent stores live in a fresh tmp + /// directory so library/profile state from prior runs doesn't bleed in. + private func makeIsolatedMainViewModel(settingsService: SettingsService = SettingsService()) -> MainViewModel { + let tempDir = FileManager.default.temporaryDirectory + .appendingPathComponent("MainVMTest-\(UUID().uuidString)", isDirectory: true) + let storage = StorageService(baseDirectory: tempDir) + let libraryStorage = LibraryStorageService(storageService: storage) + return MainViewModel( + settingsService: settingsService, + storageService: storage, + libraryStorageService: libraryStorage + ) + } + @Test("Default state") @available(macOS 14.0, iOS 17.0, visionOS 1.0, *) func testDefaultState() { - let vm = MainViewModel() + let vm = makeIsolatedMainViewModel() #expect(vm.selectedDestination == .library) #expect(vm.isInspectorVisible == false) #expect(vm.searchText == "")