Avoid using legacy test data in most (but not all) of SymbolTests.swift #1278

d-ronnqvist · 2025-08-18T13:16:01Z

Bug/issue #, if applicable:

Summary

This is a test only change to phase out some usage of the "LegacyBundle_DoNotUseInNewTests".

After seeing new code that used makeDocumentationNodeForSymbol(...) and noticing that it used "LegacyBundle_DoNotUseInNewTests" internally I decided to update those tests and reduce the usage of that legacy fixture.

While doing this I had to make several correction to most of those tests because the original makeDocumentationNodeForSymbol(...) implementation incorrectly skipped most warnings in the documentation, resulting in tests that verify 0 problems when there was in fact 2 or 3 problems with the documentation.

Dependencies

None

Testing

Nothing in particular. This isn't a user-facing change.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

~~Added~~Updated tests
Ran the ./bin/test script and it succeeded
~~Updated documentation if necessary~~ Not applicable

d-ronnqvist · 2025-08-18T13:16:58Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

@@ -53,20 +53,18 @@ class SymbolTests: XCTestCase {
                  - name: A parameter
                - Returns: Return value
                """,
-            articleContent: """
-                # Leading heading is ignored


This was incorrect. It's not ignored. There's a warning about this but the old test helper never reported that warning.

d-ronnqvist · 2025-08-18T13:17:30Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

                @Metadata {
                   @DocumentationExtension(mergeBehavior: override)
                }
                """
        )
-        XCTAssert(problems.isEmpty)
+        XCTAssertEqual(problems.count, 0, "Unexpected problems: \(problems.map(\.diagnostic.summary).sorted())")


This is technically not necessary but makes it nicer to debug these tests in the future if they break because of a new diagnostic.

d-ronnqvist · 2025-08-18T13:18:16Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift


                """
        )
-        XCTAssert(problems.isEmpty)


This was incorrect. There's should be two warnings about curating the module and curating the container type but the old test helper never reported those warnings.

d-ronnqvist · 2025-08-18T13:18:29Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

+                       "The article overrides—and removes—the abstract from the in-source documentation")
        XCTAssertNil(withArticleOverride.discussion,
-                       "The article overries the discussion.")
+                       "The article overrides the discussion.")


This is just fixing typos

d-ronnqvist · 2025-08-18T13:19:44Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

                    """
            )
-            XCTAssert(problems.isEmpty)


This was incorrect. There are two sources of warnings here:

The redundant DocumentationExtension(mergeBehavior: append) raises a warning

The cyclic curation raises two warnings.

The old test helper never reported these kinds of warnings so the test was incorrectly asserting that there were no warnings.

d-ronnqvist · 2025-08-18T13:21:28Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

+        XCTAssertEqual(problems.first?.diagnostic.range?.lowerBound.line, 14)
+        XCTAssertEqual(problems.first?.diagnostic.range?.lowerBound.column, 18)


To help test that source location are reported correctly, the new test uses a non-zero row and column for the start of the doc comment. This helps verify that locations are reported in absolute values rather than relative values.

d-ronnqvist · 2025-08-18T13:22:23Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

+        let myFunctionUSR = "s:5MyKit0A5ClassC10myFunctionyyF"
+        let (_, _, context) = try await testBundleAndContext(copying: "LegacyBundle_DoNotUseInNewTests") { url in
+            var graph = try JSONDecoder().decode(SymbolGraph.self, from: Data(contentsOf: url.appendingPathComponent("mykit-iOS.symbols.json")))
+
+            let newDocComment = self.makeLineList(
+                docComment: """
                A cool API to call.

                - Parameters:
                  - name: A parameter
                - Returns: Return value
                """,
-            articleContent: nil
-        )
+                moduleName: nil,
+                startOffset: .init(line: 0, character: 0),
+                url: URL(string: "file:///tmp/File.swift")!
+            )
+
+            // The `guard` statement` below will handle the `nil` case by failing the test and
+            graph.symbols[myFunctionUSR]?.docComment = newDocComment
+
+            let newGraphData = try JSONEncoder().encode(graph)
+            try newGraphData.write(to: url.appendingPathComponent("mykit-iOS.symbols.json"))
+        }


There was too much in this test that relied on the legacy fixture so rather than fix it I inlined the setup code from the old test helper here.

d-ronnqvist · 2025-08-18T13:22:52Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

@@ -1309,6 +1315,8 @@ class SymbolTests: XCTestCase {
                "org.swift.docc.Metadata.InvalidPageColorInDocumentationComment",
                "org.swift.docc.Metadata.InvalidTitleHeadingInDocumentationComment",
                "org.swift.docc.Metadata.InvalidRedirectedInDocumentationComment",
+
+                "org.swift.docc.unresolvedResource", // For the "test" asset that doesn't exist.


Like elsewhere, this warning should be there but the old test helper never reported this type of warning.

d-ronnqvist · 2025-08-18T13:23:42Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

-        docCommentLineOffset: Int = 0,
-        articleContent: String?,
+        docCommentLineStart: Int = 11, // an arbitrary non-zero start line
+        extensionFileContent: String?,


I renamed this argument because this parameter doesn't represent the content of an article it represents the content of an extension file. The original test helper was confusing in this regard.

d-ronnqvist · 2025-08-18T13:25:09Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

@@ -1506,79 +1514,78 @@ class SymbolTests: XCTestCase {

    // MARK: - Helpers

-    func makeDocumentationNodeForSymbol(
+    private func makeDocumentationNodeForSymbol(


There is enough special knowledge in this test file about the specifics of this test data (for example that the symbol is a function, that it has a "name" parameter, and that it has a return value) for this to be useful in other test files. To prevent other test files from accidentally using it, I made it private (same with the other helper below)

would it make sense to push that further to fileprivate? (are the uses all in this test suite?)

private is more restrictive than fileprivate (or equivalent in top-level declarations) but in this case the file only has one XCTestCase class, so it doesn't make any difference in practice.

d-ronnqvist · 2025-08-18T13:26:05Z

@swift-ci please test

d-ronnqvist · 2025-08-19T09:09:15Z

@swift-ci please test

d-ronnqvist · 2025-08-20T11:08:48Z

@swift-ci please test

d-ronnqvist · 2025-08-20T11:36:11Z

@swift-ci please test

heckj

Nice updates - as a question (not directly related to this PR) - is there a pattern of adding tests to validate warnings and the edge cases for those as they're added? Is that an intention, or kind of a side effect, of this cleanup?

heckj · 2025-08-20T19:07:02Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

@@ -1506,79 +1514,78 @@ class SymbolTests: XCTestCase {

    // MARK: - Helpers

-    func makeDocumentationNodeForSymbol(
+    private func makeDocumentationNodeForSymbol(


would it make sense to push that further to fileprivate? (are the uses all in this test suite?)

d-ronnqvist · 2025-08-21T08:17:50Z

Nice updates - as a question (not directly related to this PR) - is there a pattern of adding tests to validate warnings and the edge cases for those as they're added? Is that an intention, or kind of a side effect, of this cleanup?

I would say that it's a little bit of both.

Based on a quick regex search we have 239 places in 56 files that do some variation of XCTAssertEqual(context.problems.count, ...) or XCTAssertTrue(context.problems.isEmpty). Many of these also use "Unexpected problems: \(problems.map(\.diagnostic.summary).sorted())") as the assertion failure message so that it's easy to identify unexpected problem without needing to rerun the tests in a debugger.
I like this pattern because it means that any new warning automatically gets checked in a lot of existing tests and the person who adds the warning is forced to evaluate any potential false positives if those any of these test assertions fail.

At the same time, in this specific PR the additional warning validation is largely a side effect of cleaning up the test helper. Because the original test helper was using a legacy fixture that has lots of warnings, the test helper doesn't reported all warnings. Instead it only reported the warnings raised from merging the symbol markup with the extension file markup. This unfortunately doesn't include any "checkers" that run on that markup, doesn't include curation warnings, doesn't include link warnings, doesn't include asset warnings, etc. for that same markup. This led to these test asserting and claiming behaviors that were incorrect. By fixing the test helper so that it reported all warnings, I forced the tests to correct these inaccuracies and deal with these warnings (or correct the markup to caused these warnings).

Avoid using legacy test data in most (but not all) tests in this file

072c504

d-ronnqvist added the code cleanup Code cleanup *without* user facing changes label Aug 18, 2025

d-ronnqvist commented Aug 18, 2025

View reviewed changes

d-ronnqvist changed the title ~~Avoid using legacy test data in most (but not all) tests in SymbolTests.swift~~ Avoid using legacy test data in most (but not all) of SymbolTests.swift Aug 18, 2025

d-ronnqvist force-pushed the phase-out-legacy-fixture-in-symbol-tests branch from ca311c5 to 805b7cb Compare August 20, 2025 11:08

Merge branch 'main' into phase-out-legacy-fixture-in-symbol-tests

fec0864

d-ronnqvist force-pushed the phase-out-legacy-fixture-in-symbol-tests branch from 805b7cb to fec0864 Compare August 20, 2025 11:35

heckj reviewed Aug 20, 2025

View reviewed changes

		XCTAssertEqual(problems.first?.diagnostic.range?.lowerBound.line, 14)
		XCTAssertEqual(problems.first?.diagnostic.range?.lowerBound.column, 18)

Avoid using legacy test data in most (but not all) of SymbolTests.swift #1278

Are you sure you want to change the base?

Avoid using legacy test data in most (but not all) of SymbolTests.swift #1278

Uh oh!

Conversation

d-ronnqvist commented Aug 18, 2025

Summary

Dependencies

Testing

Checklist

Uh oh!

d-ronnqvist Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist commented Aug 18, 2025

Uh oh!

d-ronnqvist commented Aug 19, 2025

Uh oh!

d-ronnqvist commented Aug 20, 2025

Uh oh!

d-ronnqvist commented Aug 20, 2025

Uh oh!

heckj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist commented Aug 21, 2025

Uh oh!

Uh oh!

d-ronnqvist Aug 18, 2025 •

edited

Loading

d-ronnqvist Aug 18, 2025 •

edited

Loading

d-ronnqvist Aug 18, 2025 •

edited

Loading