Adapt tests to work better in Swift CI #45

d-ronnqvist · 2021-11-18T22:09:25Z

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

Summary

This updates the tests to no longer write data in NSTemporaryDirectory().

Dependencies

n/a

Testing

n/a

Checklist

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

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

d-ronnqvist · 2021-11-18T22:11:21Z

It may be good to run these tests a few times before merging. I'll run them once now to confirm that these changes work on the different platforms.

d-ronnqvist · 2021-11-18T22:11:29Z

@swift-ci please test

d-ronnqvist · 2021-11-18T23:19:53Z

Sources/SwiftDocCTestUtilities/XCTestCase+TemporaryDirectory.swift

+        fileManager: FileManager = .default
+    ) throws -> URL {
+        let bundleParentDir = Bundle(for: Self.self).bundleURL.deletingLastPathComponent()
+        let baseURL = bundleParentDir.appendingPathComponent(name.replacingWhitespaceAndPunctuation(with: "-"))


@gottesmm Does this look like a suitable base path for temp data in tests?

@shahmishal what do you think?

@shahmishal Does this base path look good or is there a better base path for temp data in tests on Swift CI?

d-ronnqvist · 2021-11-18T23:23:21Z

Package.swift

+
+        // Test utility library
+        .target(
+            name: "SwiftDocCTestUtilities",


Right now this target is only used to share one file between the two test targets but there are more code that could be moved out of SwiftDocC itself into this test target instead.

This is great! I've been wanting something like this for a while. No need to do it now- but eventually moving all of our test bundles over to this library would have a big impact on our repo size I think.

d-ronnqvist · 2021-11-19T00:03:12Z

We should wait until #44 lands and have this PR update the new tests that were added in that PR.

Tests/SwiftDocCTests/XCTestCase+LoadingTestData.swift

...wiftDocCUtilities/ArgumentParsing/ActionExtensions/ConvertAction+CommandInitialization.swift

franklinsch · 2021-11-19T11:06:58Z

Sources/SwiftDocCTestUtilities/XCTestCase+TemporaryDirectory.swift

+
+public extension XCTestCase {
+
+    @available(*, deprecated, message: "Use `createTemporaryDirectory` instead in unit tests to avoid referencing a shared location in Swift CI.")


What's blocking us from moving away from NSTemporaryDirectory everywhere? I find it odd to mark an API as deprecated which still has usages within the repo, we're going to see deprecation warnings for those.

This function only shadows calls made from within test cases. I've updated all of those, so this doesn't add any deprecation warnings.

Can we do this for FileManager.default.temporaryDirectory as well? Not sure if Swift will allow that...

No. I'm not aware of a way tp shadow a property on another type like that.

I gave this a go- it looks like you can just redefine it but you need to define it in each test framework so that the compiler knows which overload to favor. (It uses an overload in the local module over one in the imported module.)

extension FileManager { @available(*, deprecated, message: "Use `createTemporaryDirectory` instead in unit tests to avoid referencing a shared location in Swift CI.") var temporaryDirectory: URL { XCTFail("Use `createTemporaryDirectory` instead in unit tests to avoid referencing a shared location in Swift CI.") return URL(fileURLWithPath: Foundation.NSTemporaryDirectory()) } }

I just added this in this branch and actually found a couple uses:

/swift-docc/Tests/SwiftDocCUtilitiesTests/ConvertActionTests.swift:1843:58: 'temporaryDirectory' is deprecated: Use `createTemporaryDirectory` instead in unit tests to avoid referencing a shared location in Swift CI. /swift-docc/Tests/SwiftDocCUtilitiesTests/ConvertActionTests.swift:1910:64: 'temporaryDirectory' is deprecated: Use `createTemporaryDirectory` instead in unit tests to avoid referencing a shared location in Swift CI. /swift-docc/Tests/SwiftDocCTests/Converter/DocumentationConverterTests.swift:29:43: 'temporaryDirectory' is deprecated: Use `createTemporaryDirectory` instead in unit tests to avoid referencing a shared location in Swift CI.

Great. I didn't think that would work. I'll incorporate the changes. Thanks!

Sources/SwiftDocCTestUtilities/XCTestCase+TemporaryDirectory.swift

Tests/SwiftDocCTests/Infrastructure/DocumentationContext+RootPageTests.swift

Tests/SwiftDocCTests/XCTestCase+LoadingTestData.swift

ethan-kusters · 2021-12-18T00:11:55Z

Sources/SwiftDocCTestUtilities/XCTestCase+TemporaryDirectory.swift

+    ///   - pathComponents: Additional path components to add to the temporary URL.
+    ///   - fileManager: The file manager that will create the directory.
+    /// - Returns: The URL of the newly created directory.
+    func createTemporaryDirectory(pathComponents: String..., fileManager: FileManager = .default) throws -> URL {


I think all of our current uses of this just specify a single path component because they need a specifically named temporary directory. Maybe this would be easier to understand at the callsite with something like-

Suggested change

func createTemporaryDirectory(pathComponents: String..., fileManager: FileManager = .default) throws -> URL {

func createTemporaryDirectory(named directoryName: String? = nil, fileManager: FileManager = .default) throws -> URL {

ethan-kusters · 2021-12-18T00:17:43Z

Sources/SwiftDocCTestUtilities/XCTestCase+TemporaryDirectory.swift

+    /// - Returns: The URL of the newly created directory.
+    func createTemporaryDirectory(pathComponents: String..., fileManager: FileManager = .default) throws -> URL {
+        let bundleParentDir = Bundle(for: Self.self).bundleURL.deletingLastPathComponent()
+        let baseURL = bundleParentDir.appendingPathComponent(name.replacingWhitespaceAndPunctuation(with: "-"))


Should we consider creating a parent TemporarySwiftDocCTestDirectory-\(some-static-unique-process-identifier) to place the individual test directories based on the test name in? Otherwise I think it's possible we'd run into collisions in the future.

I'm imagining something like:

// Define this statically so that we don't generate a new unique base directory // every time we create a temporary directory private static let uniqueProcessIdentifier = ProcessInfo.processInfo.globallyUniqueString

Suggested change

let baseURL = bundleParentDir.appendingPathComponent(name.replacingWhitespaceAndPunctuation(with: "-"))

let baseURL = bundleParentDir

.appendingPathComponent(

"TemporarySwiftDocCTestDirectory-\(uniqueProcessIdentifier)",

isDirectory: true

)

.appendingPathComponent(

name.replacingWhitespaceAndPunctuation(with: "-"),

isDirectory: true

)

Collisions shouldn't be an issue here.

The bundleParentDir is a path inside the build directory for the specific checkout, so that should be unique if Swift CI is testing two different builds of Swift-DocC at the same time.

The name that's used in the base URL contains both the name of the test class and the test case.

Together this means that only tests in the same checkout and in the same test method will have the same base URL. Since the teardown block runs after the test method there's no risk that a base URL is removed while there are tests working on subpaths of that URL.

The first path component within each test base URL is ProcessInfo.processInfo.globallyUniqueString so two different temp directories in the same test method also wouldn't have collisions.

The name that's used in the base URL contains both the name of the test class and the test case.

Ah got it- based on the docs I was assuming that it was just the name of the test case so I'm less concerned then.

However- the collisions I was more worried about are with other items in the bundle, for example if we added a new TestDocC target and SwiftPM emitted a new directory there to hold its output. The chances of collision seem very low though since it's both the test class and test case so I think this is okay.

Sources/SwiftDocCTestUtilities/XCTestCase+TemporaryDirectory.swift

ethan-kusters · 2021-12-18T00:20:30Z

Sources/SwiftDocCTestUtilities/XCTestCase+TemporaryDirectory.swift

+
+public extension XCTestCase {
+
+    @available(*, deprecated, message: "Use `createTemporaryDirectory` instead in unit tests to avoid referencing a shared location in Swift CI.")


Can we do this for FileManager.default.temporaryDirectory as well? Not sure if Swift will allow that...

…ectories

d-ronnqvist · 2021-12-22T01:47:45Z

@swift-ci please test

This is clearer at the call site when only a single path component is specified.

d-ronnqvist · 2021-12-22T21:43:08Z

@swift-ci please test

franklinsch · 2022-01-07T16:09:39Z

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

@@ -200,7 +203,8 @@ public struct ConvertAction: Action, RecreatingContext {
        inheritDocs: Bool = false,
        experimentalEnableCustomTemplates: Bool = false,
        transformForStaticHosting: Bool,
-        hostingBasePath: String?
+        hostingBasePath: String?,
+        temporaryDirectory: URL


Nit: this is slightly odd as parameter with no default value for ConvertAction. I wouldn't expect clients to need to provide a temporary directory, since using a temporary directory is an implementation detail of ConvertAction. Should we set a default value of FileManager.default.temporaryDirectory?

I would prefer to not provide a default value for this argument to avoid mistakes where a test would forget to provide a custom value.

franklinsch

Thanks David, this looks great to me! Appreciate the thoroughness 💯

d-ronnqvist · 2022-01-08T00:59:33Z

@swift-ci please test

* Add utility to create temp directories in unit tests * Create new target for test utilities * Pass the temporary directory to ConvertAction * Re-enable one skipped test * Update new tests to use `createTemporaryDirectory` helper * Remove `createDirectoryForLastPathComponent` argument in test helper * Update test helper documentation * Prefer `FileManager.temporaryDirectory` property in ConvertAction * Move `Files` and `Folder` types into test utility target * Add additional safety checks when creating and removing temporary directories * Replace `TempFolder` test class with `createTempFolder` test function * Use XCTUnwrap instead of force unwrap in test helper * Also shadow `FileManager.temporaryDirectory` in tests * Add temp directory test helper variant with "named" argument This is clearer at the call site when only a single path component is specified. * Fix test helper syntax in disabled preview server test * Re-enable one file monitoring test

etcwilde · 2022-01-18T22:32:25Z

It looks like testNavigationTreeLargeDumpAndReadAsync is creating some noise on the Linux CI again.
https://ci.swift.org/job/swift-corelibs-foundation-PR-Linux/5230/

Test Suite 'Selected tests' started at 2022-01-18 19:29:23.951
Test Suite 'NavigatorIndexingTests' started at 2022-01-18 19:29:23.953
Test Case 'NavigatorIndexingTests.testNavigationTreeLargeDumpAndReadAsync' started at 2022-01-18 19:29:23.953
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift-docc/Tests/SwiftDocCTests/Indexing/NavigatorIndexTests.swift:250: error: NavigatorIndexingTests.testNavigationTreeLargeDumpAndReadAsync : Asynchronous wait failed - Exceeded timeout of 10.0 seconds, with unfulfilled expectations: Load the tree asynchronously.
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift-docc/Tests/SwiftDocCTests/Indexing/NavigatorIndexTests.swift:252: error: NavigatorIndexingTests.testNavigationTreeLargeDumpAndReadAsync : XCTAssertEqual failed: ("656051") is not equal to ("499671") - 
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift-docc/Tests/SwiftDocCTests/Indexing/NavigatorIndexTests.swift:253: error: NavigatorIndexingTests.testNavigationTreeLargeDumpAndReadAsync : XCTAssertTrue failed - 
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift-docc/Tests/SwiftDocCTests/Indexing/NavigatorIndexTests.swift:261: error: NavigatorIndexingTests.testNavigationTreeLargeDumpAndReadAsync : Asynchronous wait failed - Exceeded timeout of 10.0 seconds, with unfulfilled expectations: Load the tree asynchronously, again with presentation identifier.
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift-docc/Tests/SwiftDocCTests/Indexing/NavigatorIndexTests.swift:262: error: NavigatorIndexingTests.testNavigationTreeLargeDumpAndReadAsync : XCTAssertEqual failed: ("656051") is not equal to ("350365") - 
Test Case 'NavigatorIndexingTests.testNavigationTreeLargeDumpAndReadAsync' failed (47.834 seconds)
Test Suite 'NavigatorIndexingTests' failed at 2022-01-18 19:30:11.787
	 Executed 1 test, with 5 failures (0 unexpected) in 47.834 (47.834) seconds
Test Suite 'Selected tests' failed at 2022-01-18 19:30:11.787
	 Executed 1 test, with 5 failures (0 unexpected) in 47.834 (47.834) seconds
FAIL: Testing swift-docc failed

Updates swift-markdown to include: commit 97df6e2812adcf8698204ca5f0756563ef36e5c1 Author: Ethan Kusters <ekusters@apple.com> Date: Sat May 21 14:02:22 2022 -0700 Correctly set `indexInParent` in `Markup.child(at:)` (swiftlang#45) This fixes a recent regression where `Markup.child(at:)` began returning markup with incorrect metadata resulting in out-of-bounds errors. The `indexInParent` value of a child is unrelated to the parent's `indexInParent`. This was missed initially because tests were only checking for children of the first item where `indexInParent` would be 0. A new test has been added that asserts `Markup.child(at:)` returns correct values for children of nested items as well. commit 7a7c59d1160fddd23e2379ec5ee8c71df7fd0b3b Author: Ethan Kusters <ekusters@apple.com> Date: Fri May 20 18:53:24 2022 -0700 Improve performance of `Markup.child(at:)` method (swiftlang#44) Improves the performance of `Markup.child(at:)` by refactoring to removing the need to iterate over all previous elements in the child array. commit 36cf89e36e94b025dbe37d82fe2a7da3f39d4204 Author: Franklin Schrans <fschrans@apple.com> Date: Wed Mar 23 14:38:18 2022 +0100 Remove extraneous print in test (swiftlang#33) commit e50693584310a9190071c96c839485b6f2376832 Author: Ethan Kusters <ekusters@apple.com> Date: Mon Mar 21 10:42:02 2022 -0700 Publish Swift Markdown's documentation to GitHub pages (swiftlang#32) * Adopt the Swift-DocC Plugin for documentation generation * Add a script for publishing docs to GitHub pages * Add missing license headers * Move README docs to articles in the DocC catalog * Remove out-of-date documentation about `DiagnosticEngine` Swift markdown no longer includes a DiagnosticEngine so these links were failing to resolve. commit 5f10cfb030f43222a49ea34857ff07f02f1e38b0 Author: christopherweems <github@christopherweems.com> Date: Sat Mar 19 12:53:31 2022 -0400 Fix typo in documentation for `MarkupVisitor` (swiftlang#26) Co-authored-by: Christopher Weems <hello@christopherweems.com>

Updates swift-markdown to include: commit 97df6e2812adcf8698204ca5f0756563ef36e5c1 Author: Ethan Kusters <ekusters@apple.com> Date: Sat May 21 14:02:22 2022 -0700 Correctly set `indexInParent` in `Markup.child(at:)` (#45) This fixes a recent regression where `Markup.child(at:)` began returning markup with incorrect metadata resulting in out-of-bounds errors. The `indexInParent` value of a child is unrelated to the parent's `indexInParent`. This was missed initially because tests were only checking for children of the first item where `indexInParent` would be 0. A new test has been added that asserts `Markup.child(at:)` returns correct values for children of nested items as well. commit 7a7c59d1160fddd23e2379ec5ee8c71df7fd0b3b Author: Ethan Kusters <ekusters@apple.com> Date: Fri May 20 18:53:24 2022 -0700 Improve performance of `Markup.child(at:)` method (#44) Improves the performance of `Markup.child(at:)` by refactoring to removing the need to iterate over all previous elements in the child array. commit 36cf89e36e94b025dbe37d82fe2a7da3f39d4204 Author: Franklin Schrans <fschrans@apple.com> Date: Wed Mar 23 14:38:18 2022 +0100 Remove extraneous print in test (#33) commit e50693584310a9190071c96c839485b6f2376832 Author: Ethan Kusters <ekusters@apple.com> Date: Mon Mar 21 10:42:02 2022 -0700 Publish Swift Markdown's documentation to GitHub pages (#32) * Adopt the Swift-DocC Plugin for documentation generation * Add a script for publishing docs to GitHub pages * Add missing license headers * Move README docs to articles in the DocC catalog * Remove out-of-date documentation about `DiagnosticEngine` Swift markdown no longer includes a DiagnosticEngine so these links were failing to resolve. commit 5f10cfb030f43222a49ea34857ff07f02f1e38b0 Author: christopherweems <github@christopherweems.com> Date: Sat Mar 19 12:53:31 2022 -0400 Fix typo in documentation for `MarkupVisitor` (#26) Co-authored-by: Christopher Weems <hello@christopherweems.com>

Includes the following changes: commit 7e704622ef98b3cc9233283b6298ed61806ddb43 Author: Ethan Kusters <ekusters@apple.com> Date: Sat May 21 14:02:22 2022 -0700 Correctly set `indexInParent` in `Markup.child(at:)` (swiftlang#45) This fixes a recent regression where `Markup.child(at:)` began returning markup with incorrect metadata resulting in out-of-bounds errors. The `indexInParent` value of a child is unrelated to the parent's `indexInParent`. This was missed initially because tests were only checking for children of the first item where `indexInParent` would be 0. A new test has been added that asserts `Markup.child(at:)` returns correct values for children of nested items as well. commit 5cc5656732244237066214bc976570f67cfe47b7 Author: Ethan Kusters <ekusters@apple.com> Date: Fri May 20 18:53:24 2022 -0700 Improve performance of `Markup.child(at:)` method (swiftlang#44) Improves the performance of `Markup.child(at:)` by refactoring to removing the need to iterate over all previous elements in the child array.

Includes the following changes: commit 7e704622ef98b3cc9233283b6298ed61806ddb43 Author: Ethan Kusters <ekusters@apple.com> Date: Sat May 21 14:02:22 2022 -0700 Correctly set `indexInParent` in `Markup.child(at:)` (#45) This fixes a recent regression where `Markup.child(at:)` began returning markup with incorrect metadata resulting in out-of-bounds errors. The `indexInParent` value of a child is unrelated to the parent's `indexInParent`. This was missed initially because tests were only checking for children of the first item where `indexInParent` would be 0. A new test has been added that asserts `Markup.child(at:)` returns correct values for children of nested items as well. commit 5cc5656732244237066214bc976570f67cfe47b7 Author: Ethan Kusters <ekusters@apple.com> Date: Fri May 20 18:53:24 2022 -0700 Improve performance of `Markup.child(at:)` method (#44) Improves the performance of `Markup.child(at:)` by refactoring to removing the need to iterate over all previous elements in the child array.

d-ronnqvist commented Nov 18, 2021

View reviewed changes

d-ronnqvist commented Nov 19, 2021

View reviewed changes

Tests/SwiftDocCTests/XCTestCase+LoadingTestData.swift Outdated Show resolved Hide resolved

ethan-kusters reviewed Nov 19, 2021

View reviewed changes

...wiftDocCUtilities/ArgumentParsing/ActionExtensions/ConvertAction+CommandInitialization.swift Outdated Show resolved Hide resolved

franklinsch reviewed Nov 19, 2021

View reviewed changes

d-ronnqvist added 8 commits December 13, 2021 10:39

Add utility to create temp directories in unit tests

65fa3d5

Create new target for test utilities

fb6fa6e

Pass the temporary directory to ConvertAction

b645298

Re-enable one skipped test

b7e969b

Update new tests to use createTemporaryDirectory helper

777319f

Remove createDirectoryForLastPathComponent argument in test helper

0240e39

Update test helper documentation

af38caa

Prefer FileManager.temporaryDirectory property in ConvertAction

8c614df

d-ronnqvist force-pushed the ci-test-separation branch from 145e5fb to 8c614df Compare December 13, 2021 20:30

ethan-kusters reviewed Dec 18, 2021

View reviewed changes

d-ronnqvist added 5 commits December 21, 2021 12:57

Merge branch 'main' into ci-test-separation

2566321

Move Files and Folder types into test utility target

5d4ce3e

Add additional safety checks when creating and removing temporary dir…

db26074

…ectories

Replace TempFolder test class with createTempFolder test function

a3035e2

Use XCTUnwrap instead of force unwrap in test helper

cc3d57d

d-ronnqvist force-pushed the ci-test-separation branch from adfe92c to cc3d57d Compare December 22, 2021 01:46

Merge branch 'main' into ci-test-separation

3105c5c

d-ronnqvist added 4 commits December 22, 2021 13:42

Also shadow FileManager.temporaryDirectory in tests

0436ad5

Add temp directory test helper variant with "named" argument

d71cc38

This is clearer at the call site when only a single path component is specified.

Fix test helper syntax in disabled preview server test

1e5de9f

Re-enable one file monitoring test

4dac6a4

franklinsch reviewed Jan 7, 2022

View reviewed changes

franklinsch approved these changes Jan 7, 2022

View reviewed changes

Merge branch 'main' into ci-test-separation

745e09a

d-ronnqvist merged commit 3f6b63b into swiftlang:main Jan 8, 2022

This was referenced Jan 11, 2022

[5.6] Adapt tests to work better in Swift CI (#45) #68

Closed

[5.6] Adapt tests to work better in Swift CI (#45) #69

Merged

d-ronnqvist deleted the ci-test-separation branch January 12, 2022 18:09


		public extension XCTestCase {

		@available(*, deprecated, message: "Use `createTemporaryDirectory` instead in unit tests to avoid referencing a shared location in Swift CI.")

	func createTemporaryDirectory(pathComponents: String..., fileManager: FileManager = .default) throws -> URL {
	func createTemporaryDirectory(named directoryName: String? = nil, fileManager: FileManager = .default) throws -> URL {

Adapt tests to work better in Swift CI #45

Adapt tests to work better in Swift CI #45

Uh oh!

Conversation

d-ronnqvist commented Nov 18, 2021

Summary

Dependencies

Testing

Checklist

Uh oh!

d-ronnqvist commented Nov 18, 2021

Uh oh!

d-ronnqvist commented Nov 18, 2021

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 Nov 19, 2021

Uh oh!

Uh oh!

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!

ethan-kusters Dec 22, 2021 • 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!

ethan-kusters Dec 22, 2021 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist commented Dec 22, 2021

Uh oh!

d-ronnqvist commented Dec 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franklinsch left a comment

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist commented Jan 8, 2022

Uh oh!

etcwilde commented Jan 18, 2022

Uh oh!

ethan-kusters Dec 22, 2021 •

edited

Loading

ethan-kusters Dec 22, 2021 •

edited

Loading