Skip to content

Improve test hygiene in ConvertSubcommandTests #934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

QuietMisdreavus
Copy link
Contributor

Bug/issue #, if applicable: None

Summary

This PR adds some setup and teardown for the tests in SwiftDocCUtilitiesTests.ConvertSubcommandTests to clean up the process environment and working directory before and after each test. This resolves some test flakiness that depended on specific environmental conditions. In particular, it resolves testParameterValidationFeatureFlag failing when tests are run sequentially, and resolves testTreatWarningAsError failing when a symlink is present in the Swift-DocC directory tree.

Dependencies

None

Testing

Steps:

  1. touch test && ln -s test test2
  2. swift test (Important: not bin/test, as the test only failed when run sequentially due to unclean environment)
  3. Ensure that all tests pass.
  4. rm test test2 (to clean up your repo state)

Checklist

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

  • [ n/a ] Added tests
  • Ran the ./bin/test script and it succeeded
  • [ n/a ] Updated documentation if necessary

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

FileManager.default.changeCurrentDirectoryPath(temporaryDirectory.path())
addTeardownBlock {
FileManager.default.changeCurrentDirectoryPath(priorWorkingDirectory)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to set the default template here as well? It seems to me like all tests except testOptionsValidation and testTransformForStaticHostingFlagWithoutHTMLTemplate want the default template to be set but that it's easy forget.

For example, are these tests expected to set the default template or not?

  • testEmitLMDBIndex
  • testExperimentalEnableCustomTemplatesFlag
  • testExperimentalEnableDeviceFrameSupportFlag
  • testExperimentalEnableExternalLinkSupportFlag
  • testExperimentalEnableOverloadedSymbolPresentation
  • testParameterValidationFeatureFlag
Suggested change
}
}
setTemplateEnvironmentVariable(testTemplateURL.path)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably the best. I think i wasn't sure how to manage the fact that testOptionsValidation sets the default template during the test, but i think setting it to a reasonable default and letting it mess with it later isn't a problem. I'll make that change.

@QuietMisdreavus
Copy link
Contributor Author

/home/build-user/swift-docc/Tests/SwiftDocCUtilitiesTests/ArgumentParsing/ConvertSubcommandTests.swift:34:75: error: cannot call value of non-function type 'String'
 32 |         let priorWorkingDirectory = FileManager.default.currentDirectoryPath
 33 |         let temporaryDirectory = try createTemporaryDirectory()
 34 |         FileManager.default.changeCurrentDirectoryPath(temporaryDirectory.path())
    |                                                                           `- error: cannot call value of non-function type 'String'

It looks like the Linux CI is running Swift 5.8; was the path() method added in 5.9?

this fixes an issue when tests are run sequentially;
testParameterValidationFeatureFlag (added in swiftlang#897) can fail if run after
testOptionsValidation, because it sets `DOCC_HTML_DIR` to a temporary
directory, which is deleted after the test exits. by resetting the
environment variable in a defer block, we can restore a proper
environment for later tests.
this ensures that the tests aren't affected by the machine environment,
for example if a gitignore'd path contains a symlink
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@d-ronnqvist
Copy link
Contributor

It looks like the Linux CI is running Swift 5.8; was the path() method added in 5.9?

You're right 👀

SWIFT_VERSION=5.8.1
SWIFT_WEBROOT=https://download.swift.org
SWIFT_TAG=swift-5.8.1-RELEASE
SWIFT_BRANCH=swift-5.8.1-release

This seems to have been like this for at least a month, but possibly for longer.

Copy link
Contributor

@mayaepps mayaepps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing these!

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}


override func setUpWithError() throws {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we combine these so that we only have one set up implementation?

The order of the different set up methods are documented but it could be easier to follow the implementation if all setup was in one method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can move the existing setUp() code into setUpWithError(); i agree that that would make it easier to follow.

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus QuietMisdreavus merged commit a76ddce into swiftlang:main Jun 4, 2024
@QuietMisdreavus QuietMisdreavus deleted the fix-tests branch June 4, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants