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.

Sign up for GitHub

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
Merged

Improve test hygiene in ConvertSubcommandTests #934

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
971db9c
unset template environment after testOptionsValidation exits
QuietMisdreavus May 30, 2024
4437b81
reset the environment for all ConvertSubcommandTests
QuietMisdreavus May 31, 2024
5ea19f8
set pwd to a temporary directory
QuietMisdreavus May 31, 2024
75e3d61
change path() call to path property to fix Linux CI
QuietMisdreavus May 31, 2024
1d54fbe
review: set a default template dir for all tests
QuietMisdreavus May 31, 2024
493a3e7
review: move all setup code into setUpWithError
QuietMisdreavus Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 27 additions & 24 deletions Tests/SwiftDocCUtilitiesTests/ArgumentParsing/ConvertSubcommandTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,34 @@ class ConvertSubcommandTests: XCTestCase {

private let testTemplateURL = Bundle.module.url(
forResource: "Test Template", withExtension: nil, subdirectory: "Test Resources")!

override func setUp() {

override func setUpWithError() throws {
// By default, run all tests in a temporary directory to ensure that they are not affected
// by the machine environment.
let priorWorkingDirectory = FileManager.default.currentDirectoryPath
let temporaryDirectory = try createTemporaryDirectory()
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.


// By default, send all warnings to `.none` instead of filling the
// test console output with unrelated messages.
Docc.Convert._errorLogHandle = .none

// Set the documentation template to a well-defined default so that options parsing isn't
// affected by other tests' changing it.
let existingTemplate = ProcessInfo.processInfo.environment[TemplateOption.environmentVariableKey]
SetEnvironmentVariable(TemplateOption.environmentVariableKey, testTemplateURL.path)
addTeardownBlock {
if let existingTemplate = existingTemplate {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, existingTemplate)
} else {
UnsetEnvironmentVariable(TemplateOption.environmentVariableKey)
}
}
}

func testOptionsValidation() throws {
// create source bundle directory
let sourceURL = try createTemporaryDirectory(named: "documentation")
Expand All @@ -34,7 +55,7 @@ class ConvertSubcommandTests: XCTestCase {
// create template dir
let rendererTemplateDirectory = try createTemporaryDirectory()
try "".write(to: rendererTemplateDirectory.appendingPathComponent("index.html"), atomically: true, encoding: .utf8)

// Tests a single input.
do {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, rendererTemplateDirectory.path)
Expand Down Expand Up @@ -137,8 +158,6 @@ class ConvertSubcommandTests: XCTestCase {
}

func testDefaultCurrentWorkingDirectory() {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, testTemplateURL.path)

XCTAssertTrue(
FileManager.default.changeCurrentDirectoryPath(testBundleURL.path),
"The test env is invalid if the current working directory is not set to the current working directory"
Expand All @@ -159,7 +178,6 @@ class ConvertSubcommandTests: XCTestCase {
// Test throws on non-existing parent folder.
for outputOption in ["-o", "--output-path"] {
for path in ["/tmp/output", "/tmp", "/"] {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, testTemplateURL.path)
XCTAssertThrowsError(try Docc.Convert.parse([
outputOption, fakeRootPath + path,
testBundleURL.path,
Expand All @@ -169,7 +187,6 @@ class ConvertSubcommandTests: XCTestCase {
}

func testAnalyzerIsTurnedOffByDefault() throws {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, testTemplateURL.path)
let convertOptions = try Docc.Convert.parse([
testBundleURL.path,
])
Expand All @@ -178,8 +195,6 @@ class ConvertSubcommandTests: XCTestCase {
}

func testInfoPlistFallbacks() throws {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, testTemplateURL.path)

// Default to nil when not passed
do {
let convertOptions = try Docc.Convert.parse([
Expand Down Expand Up @@ -226,8 +241,6 @@ class ConvertSubcommandTests: XCTestCase {
// Deprecating the test silences the deprecation warning when running the tests. It doesn't skip the test.
@available(*, deprecated)
func testAdditionalSymbolGraphFiles() throws {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, testTemplateURL.path)

// Default to [] when not passed
do {
let convertOptions = try Docc.Convert.parse([
Expand Down Expand Up @@ -291,8 +304,6 @@ class ConvertSubcommandTests: XCTestCase {
}

func testIndex() throws {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, testTemplateURL.path)

let convertOptions = try Docc.Convert.parse([
testBundleURL.path,
"--index",
Expand All @@ -319,8 +330,6 @@ class ConvertSubcommandTests: XCTestCase {
}

func testWithoutBundle() throws {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, testTemplateURL.path)

let convertOptions = try Docc.Convert.parse([
"--fallback-display-name", "DisplayName",
"--fallback-bundle-identifier", "com.example.test",
Expand Down Expand Up @@ -444,10 +453,7 @@ class ConvertSubcommandTests: XCTestCase {
let rendererTemplateDirectory = try createTemporaryDirectory()
try "".write(to: rendererTemplateDirectory.appendingPathComponent("index.html"), atomically: true, encoding: .utf8)
SetEnvironmentVariable(TemplateOption.environmentVariableKey, rendererTemplateDirectory.path)
defer {
UnsetEnvironmentVariable(TemplateOption.environmentVariableKey)
}


let dependencyDir = try createTemporaryDirectory()
.appendingPathComponent("SomeDependency.doccarchive", isDirectory: true)
let fileManager = FileManager.default
Expand Down Expand Up @@ -513,7 +519,7 @@ class ConvertSubcommandTests: XCTestCase {

func testTransformForStaticHostingFlagWithoutHTMLTemplate() throws {
UnsetEnvironmentVariable(TemplateOption.environmentVariableKey)

// Since there's no custom template set (and relative HTML template lookup isn't
// supported in the test harness), we expect `transformForStaticHosting` to
// be false in every possible scenario of the flag, even when explicitly requested.
Expand Down Expand Up @@ -546,8 +552,6 @@ class ConvertSubcommandTests: XCTestCase {
}

func testTransformForStaticHostingFlagWithHTMLTemplate() throws {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, testTemplateURL.path)

// Since we've provided an HTML template, we expect `transformForStaticHosting`
// to be true by default, and when explicitly requested. It should only be false
// when `--no-transform-for-static-hosting` is passed.
Expand Down Expand Up @@ -580,7 +584,6 @@ class ConvertSubcommandTests: XCTestCase {
}

func testTreatWarningAsError() throws {
SetEnvironmentVariable(TemplateOption.environmentVariableKey, testTemplateURL.path)
do {
// Passing no argument should default to the current working directory.
let convert = try Docc.Convert.parse([])
Expand Down