Skip to content
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

Remove unneeded print statements #101

Merged
merged 2 commits into from
Mar 29, 2022
Merged

Conversation

talzag
Copy link
Contributor

@talzag talzag commented Mar 11, 2022

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

Summary

The test LogHandleTests.testFlushesStandardOutput() fails when run outside of Xcode because stdout is written to by other tests and types. This was breaking the assumption that stdout would be empty when it was written to by the test. This PR removes several print statements from the tests that have no effect on the tests, and changes the CoverageAction type to write to a provided log handle rather than print directly to stdout.

Dependencies

None.

Testing

Steps:

  1. Download the latest 5.6 toolchain snapshot from swift.org.
  2. Install the toolchain and cd </path/to/swift-docc>
  3. Run xcrun --toolchain org.swift.<toolchain ID> swift test (i.e. xcrun --toolchain org.swift.56202203021a swift test)
    • You can get the bundle ID of a toolchain on macOS with the command /usr/libexec/PlistBuddy -c "Print CFBundleIdentifier:" /Library/Developer/Toolchains/<toolchain>/Info.plist
  4. Verify that no test failures are reported.
  5. Verify that the output is formatted as expected when run with the --experimental-documentation-coverage flag (i.e. swift run docc convert --experimental-documentation-coverage --level=brief /path/to/docs.docc)

Checklist

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

@talzag
Copy link
Contributor Author

talzag commented Mar 11, 2022

@swift-ci please test

@talzag
Copy link
Contributor Author

talzag commented Mar 11, 2022

@swift-ci Please test Linux platform

@talzag
Copy link
Contributor Author

talzag commented Mar 11, 2022

@swift-ci Please test macOS platform

@daniel-grumberg
Copy link
Contributor

I am confused. What has happened with the last two commits? Did you realize that change wasn't needed?

@talzag
Copy link
Contributor Author

talzag commented Mar 11, 2022

That’s correct. Purging stdout and stderr wasn’t necessary for the tests to pass, and fpurge is only available on BSD systems.

@daniel-grumberg
Copy link
Contributor

You need to rebase this to get the latest stuff from main. While you are doing this, it would be good to remove the the purging commit and the revert of that one so the history is a bit cleaner.

@daniel-grumberg
Copy link
Contributor

Apart from that looks fine.

@talzag talzag force-pushed the flush-output-test branch from e280a8a to 513ef3b Compare March 11, 2022 19:46
@talzag
Copy link
Contributor Author

talzag commented Mar 11, 2022

@swift-ci please test

@franklinsch
Copy link
Contributor

franklinsch commented Mar 14, 2022

@talzag Does this resolve the issue of Swift-DocC tests failing in Swift CI during toolchain builds, and if so, why? Also, why did the test start failing in Swift CI; was it due to a change in another component in Swift?

@talzag
Copy link
Contributor Author

talzag commented Mar 16, 2022

@franklinsch I’m actually not sure if this is the issue that caused Swift-DocC tests to fail in the toolchain pipelines. In the radar @d-ronnqvist pointed out that none of the tests are marked as failing. However, I was seeing this test fail every time I ran the tests from the command line.

I don’t think this was caused by a change in a Swift component because I can reproduce the failure with the 5.6 snapshot toolchain and with the toolchain in my installed Xcode.

@talzag talzag force-pushed the flush-output-test branch from 513ef3b to 1a830f9 Compare March 16, 2022 00:23
@talzag
Copy link
Contributor Author

talzag commented Mar 16, 2022

@swift-ci please test

@talzag
Copy link
Contributor Author

talzag commented Mar 16, 2022

@swift-ci please test

@talzag talzag force-pushed the flush-output-test branch from fe6ca0a to f53b18b Compare March 18, 2022 16:24
@icanzilb
Copy link
Contributor

Hey @talzag, I'm wondering if this PR title could be improved to be more specific to the code changes it contains. The title says "ensure the stdout is empty" but I'm not sure how the code changes do that. Maybe rename to something like "Remove unneeded prints" or so? This is minutia but would be helpful when reviewing git history

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Mar 21, 2022

Encountering this bug too when I recently tested swift-docc in terminal with swift test
But strangely if I run ./bin/test, it will pass the tests.
The only difference is ./bin/test will call swift test --parallel instead of swift test
cc @talzag

@talzag
Copy link
Contributor Author

talzag commented Mar 21, 2022

Hey @icanzilb that’s a good point. I’ll update the title to be clearer about what this PR is changing.

@talzag talzag changed the title Ensure stdout is empty before tests Remove unneeded print statements Mar 21, 2022
@talzag
Copy link
Contributor Author

talzag commented Mar 21, 2022

@Kyle-Ye good catch! When tests are run in parallel, a process is spawned for each test class:

Test parallelization occurs by distributing the test classes in a target across multiple runner processes.

Each test class has it’s own stdin and stdout, so other tests can’t interfere with the LogHandleTests by writing to stdout.

@talzag talzag force-pushed the flush-output-test branch from f53b18b to 3a65d18 Compare March 24, 2022 15:56
@talzag
Copy link
Contributor Author

talzag commented Mar 24, 2022

@swift-ci please test

@talzag talzag force-pushed the flush-output-test branch from 3a65d18 to 7cee2a5 Compare March 29, 2022 16:16
@talzag
Copy link
Contributor Author

talzag commented Mar 29, 2022

@swift-ci please test

@talzag talzag merged commit d027bbb into swiftlang:main Mar 29, 2022
@talzag talzag deleted the flush-output-test branch March 29, 2022 16:24
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.

5 participants