Deprecate two-step documentation context creation #1059

d-ronnqvist · 2024-10-15T18:11:47Z

Bug/issue #, if applicable: rdar://136208312

Summary

This is the fifth of many changes to redefine how documentation contexts are created.

This PR deprecates the two-step context creation process (first create a context, then register a context-data-provider with it) and the various types used in this two-step process:

DocumentationConverter and DocumentationConverterProtocol
DocumentationContextDataProvider and DocumentationContextDataProviderDelegate
DocumentationWorkspace
DocumentationWorkspaceDataProvider
- GeneratedDataProvider
- PrebuiltLocalFileSystemDataProvider
- using LocalFileSystemDataProvider as a DocumentationWorkspaceDataProvider

Most of these changes are about updating tests to use discover inputs first and initialize context with their inputs:

Sources
 23 files changed, 112 insertions(+), 93 deletions(-)
Tests
 36 files changed, 407 insertions(+), 710 deletions(-)

Dependencies

This builds on the changes from #1057 (Pass documentation context its only bundle during initialization)

This diff shows the changes that are new in this PR.

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~~ The next PR in this series will update the contributor documentation to reflect these changes.

d-ronnqvist · 2024-10-15T18:29:45Z

@swift-ci please test

…iew cancellation "" (#1062) * Revert "Revert "Define commands as asynchronous and use Task for preview canc…" This reverts commit 61ce635. * Workaround miscompilation in `ConvertAction.perform(logHandle:)` * Temporarily skip until `testCancelsConversion()` until #1059 is merged * Change private `DiagnosticEngine.filter` closure to a private function

…eContext`

Also, remove no longer used `configureContext` parameter

d-ronnqvist · 2024-10-24T11:58:41Z

@swift-ci please test

d-ronnqvist · 2024-10-24T12:11:24Z

@swift-ci please test

patshaughnessy

Great work!! 👏

patshaughnessy · 2024-10-24T18:00:16Z

Sources/SwiftDocC/Infrastructure/DocumentationConverter.swift

@@ -37,6 +38,7 @@ public protocol DocumentationConverterProtocol {
 ///
 /// You can also configure the documentation converter to emit extra metadata such as linkable entities and indexing records
 /// information.
+@available(*, deprecated, message: "This deprecated API will be removed after 6.2 is released")


I noticed a comment near ConvertActionConverter.convert.recordProblem mentions DocumentationConverter - we might want to remove or update that:

These problems only represent unexpected thrown errors and aren't particularly user-facing but because
DocumentationConverter.convert(outputConsumer:) emits them as diagnostics we do the same here.

I rephrased this comment in 84b0dc0 to add a future action to change it while still mentioning that this current behavior came from DocumentationConverter in case the reader wonders why the code does this today.

patshaughnessy · 2024-10-24T18:10:33Z

Sources/SwiftDocCUtilities/Action/Actions/Convert/ConvertAction.swift

@@ -50,9 +50,6 @@ public struct ConvertAction: AsyncAction {
    ///   - buildIndex: Whether or not the convert action should emit an LMDB representation of the navigator index.
    /// 
    ///     A JSON representation is built and emitted regardless of this value.
-    ///   - workspace: A provided documentation workspace. Creates a new empty workspace if value is `nil`


Nice to see all this simplification!

patshaughnessy · 2024-10-24T18:11:56Z

Tests/SwiftDocCTests/Converter/DocumentationConverterTests.swift

@@ -12,6 +12,7 @@
 import XCTest
 @testable import SwiftDocC

+@available(*, deprecated, message: "This deprecated API will be removed after 6.2 is released")


Interesting I've never see a unit test deprecated before. Does Xcode or SPM run these as usual? Is there an option to skip them? Do we need deprecation warnings during test runs?

Ah. I usually copy and paste a template comment about this. I don't know why I didn't do that here.

Both Xcode and SwiftPM runs deprecated tests like this without skipping them and without raising deprecation warnings. The reason for marking a test as deprecated is that it allows the test to use other deprecated API without causing deprecation warnings.

I'll find that comment and add it to all the newly deprecated tests.

I added a comment in 0d28c07 to all the deprecated test (that didn't already have it) that explains that the deprecation only silenced other deprecation warnings and that it doesn't skip those tests.

patshaughnessy · 2024-10-24T18:14:32Z

Tests/SwiftDocCTests/Infrastructure/BundleDiscoveryTests.swift


-        XCTAssertEqual(bundles.count, 1)
-        guard let bundle = bundles.first else { return }
+        let fileSystem = try TestFileSystem(folders: [catalog])


Aside from being easier to read and simpler, are these tests now using the in-memory file system instead of an actual temporary directory? And are they faster?

Yes, but it's not going to be all that noticeable on the grand scheme of things.

If I look only at the tests in this file, we can see that:

testBundleDiscoveryOptions and testBundleFormat are about 2x-3x faster

testFirstBundle and testLoadComplexWorkspace are the same (because they're deprecated tests that haven't changed)

testCustomTemplatesFound, testNoCustomTemplates, testNoInfoPlist, and testThemeSettingsFound are too fast to register.

Test name on main on this branch

testBundleDiscoveryOptions 0.015 s 0.007 s

testBundleFormat 0.064 s 0.021 s

testCustomTemplatesFound 0.018 s 0.000 s

testFirstBundle 0.021 s 0.021 s

testLoadComplexWorkspace 0.027 s 0.026 s

testNoCustomTemplates 0.013 s 0.000 s

testNoInfoPlist 0.011 s 0.000 s

testThemeSettingsFound 0.016 s 0.000 s

Across those 8 tests this saves about 100ms (0.184 s on main compared to 0.088 s on this branch).

That said, across the entire test suite the difference is still less than a second because very few tests changed to using an in-memory file system and each test is only a few milliseconds speedup.

patshaughnessy · 2024-10-24T18:18:00Z

Tests/SwiftDocCUtilitiesTests/XCTestCase+LoadingData.swift

+        otherFileSystemDirectories: [Folder] = [],
+        configuration: DocumentationContext.Configuration = .init()
+    ) throws -> (DocumentationBundle, DocumentationContext) {
+        let fileSystem = try TestFileSystem(folders: [catalog] + otherFileSystemDirectories)


Repeat the previous comment: Are many of our tests now faster, avoiding actually touching the file system?

Great to see this centralized utility used so often in the tests!

Yes, some tests are a few milliseconds faster but the impact on the entire test suite is less than a second.

…s and add FIXME to change this in the future

d-ronnqvist · 2024-10-25T08:26:27Z

@swift-ci please test

d-ronnqvist added the code cleanup Code cleanup *without* user facing changes label Oct 15, 2024

d-ronnqvist mentioned this pull request Oct 15, 2024

Update contributor documentation to reflect changes in input discovery #1060

Merged

2 tasks

This was referenced Oct 16, 2024

Add support for custom scripts #1056

Open

Revert "Revert "Define commands as asynchronous and use Task for preview cancellation "" #1062

Merged

d-ronnqvist added a commit that referenced this pull request Oct 18, 2024

Temporarily skip until testCancelsConversion() until #1059 is merged

79488db

d-ronnqvist force-pushed the deprecate-workspace branch from 9fa9616 to 45a91aa Compare October 21, 2024 12:01

This was referenced Oct 22, 2024

Ensure that bundle identifiers are valid URL host components #1069

Merged

Pass documentation context its only bundle during initialization #1057

Merged

d-ronnqvist force-pushed the deprecate-workspace branch from 45a91aa to 83dfb18 Compare October 23, 2024 12:34

d-ronnqvist mentioned this pull request Oct 23, 2024

Fix occasional flakiness in preview action cancellation test #1070

Merged

2 tasks

d-ronnqvist added 18 commits October 24, 2024 13:48

Deprecate `DocumentationConverter

b9a3b8a

Update DataProvider protocol to a be general purpose

6eaf217

Update DiagnosticConsoleWriter to use DataProvider protocol

0dd80c7

Update LinkResolver to use DataProvider protocol

4caf0b6

Update one test helper to use InputProvider to discover inputs

6e5cb19

Update tests to load inputs using test helpers

476070a

Avoid loading same bundle twice in same test

73465e7

Avoid creating unused workspace and context in tests

ffc41a9

Use InMemoryDataProvider instead of custom type in test

e277b24

Avoid force try in one test

01e02e1

Move additional global checks to context configuration

0ac31ea

Avoid copying the test bundle when the catalog content isn't modified

1c8252a

Update ConvertActionIndexerTests to use InputProvider to discover inputs

e2932f8

Avoid using _legacyDataProvider in tests

9a01653

Update tests to pass link dependency files instead of using `configur…

4b827aa

…eContext`

Update final test helper to use InputProvider to discover inputs

600e623

Also, remove no longer used `configureContext` parameter

Use test file system for empty test bundle

7e19a29

Use test helper for setting feature flags

dce5adf

d-ronnqvist added 3 commits October 24, 2024 13:51

Deprecate DocumentationWorkspace

59d6a19

Update BundleDiscoveryTests to use test helpers for input discovery

27d0685

Remove unused data provider parameter from convert action initializer

33eafe1

d-ronnqvist force-pushed the deprecate-workspace branch from 83dfb18 to b8c5622 Compare October 24, 2024 11:53

d-ronnqvist added 9 commits October 24, 2024 14:09

Stop conforming test file system to workspace data provider

373246f

Deprecate GeneratedDataProvider

9f39d67

Deprecate PrebuiltLocalFileSystemDataProvider

d372098

Remove unused SymbolGraphLoader initializer

7b32c5d

Indicate in deprecated tests when they can be removed

bdaf112

Remove out-of-date deprecation warning for already removed code

9a4f86a

Deprecate DocumentationWorkspaceDataProvider

5f5c960

Deprecate DocumentationContextDataProvider

9c2d754

Deprecate additional multi-bundle-related properties

d16f146

d-ronnqvist force-pushed the deprecate-workspace branch from b8c5622 to d16f146 Compare October 24, 2024 12:09

Merge branch 'main' into deprecate-workspace

bc9a2b5

patshaughnessy approved these changes Oct 24, 2024

View reviewed changes

d-ronnqvist added 2 commits October 25, 2024 10:14

Add comments to deprecated tests explaining that they aren't skipped

0d28c07

Update comment about why unexpected errors and turned into diagnostic…

84b0dc0

…s and add FIXME to change this in the future

d-ronnqvist merged commit ed492be into swiftlang:main Oct 25, 2024
2 checks passed

d-ronnqvist deleted the deprecate-workspace branch October 25, 2024 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate two-step documentation context creation #1059

Deprecate two-step documentation context creation #1059

d-ronnqvist commented Oct 15, 2024 •

edited

Loading

d-ronnqvist commented Oct 15, 2024

d-ronnqvist commented Oct 24, 2024

d-ronnqvist commented Oct 24, 2024

patshaughnessy left a comment

patshaughnessy Oct 24, 2024

d-ronnqvist Oct 25, 2024

patshaughnessy Oct 24, 2024

patshaughnessy Oct 24, 2024

d-ronnqvist Oct 25, 2024

d-ronnqvist Oct 25, 2024

patshaughnessy Oct 24, 2024

d-ronnqvist Oct 25, 2024

patshaughnessy Oct 24, 2024

d-ronnqvist Oct 25, 2024

d-ronnqvist commented Oct 25, 2024

Test name	on main	on this branch
`testBundleDiscoveryOptions`	0.015 s	0.007 s
`testBundleFormat`	0.064 s	0.021 s
`testCustomTemplatesFound`	0.018 s	0.000 s
`testFirstBundle`	0.021 s	0.021 s
`testLoadComplexWorkspace`	0.027 s	0.026 s
`testNoCustomTemplates`	0.013 s	0.000 s
`testNoInfoPlist`	0.011 s	0.000 s
`testThemeSettingsFound`	0.016 s	0.000 s

Deprecate two-step documentation context creation #1059

Deprecate two-step documentation context creation #1059

Conversation

d-ronnqvist commented Oct 15, 2024 • edited Loading

Summary

Dependencies

Testing

Checklist

d-ronnqvist commented Oct 15, 2024

d-ronnqvist commented Oct 24, 2024

d-ronnqvist commented Oct 24, 2024

patshaughnessy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-ronnqvist commented Oct 25, 2024

d-ronnqvist commented Oct 15, 2024 •

edited

Loading