Remove long disabled integration test

[d-ronnqvist]

Bug/issue #, if applicable:

Summary

This removes an integration test—and code only used by that test—that's been disabled since the initial open source release of Swift-DocC.

The original reason for disabling this test was that it makes real network requests to a local preview server. This behavior made the test unsuitable for running in a CI environment because its effect couldn't be isolated. Since the purpose of the test is to test real network request to the local preview server, there's no reasonable expectation for reenabling this test.

Dependencies

None

Testing

None.

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

[mayaepps]

Can this test be rewritten without the network requests? From the comments it seems like the test was meant to ensure the preview request recovered after a failed convert attempt, which seems like behavior it would still be nice to test.

[d-ronnqvist]

Can this test be rewritten without the network requests? From the comments it seems like the test was meant to ensure the preview request recovered after a failed convert attempt, which seems like behavior it would still be nice to test.

Assuming that you're asking about testWatchRecoversAfterConversionErrors(); it could probably be rewritten to work but I didn't see much value in keeping code that's been commented out for over 2.5 years. I can add it back if you prefer.

From what I understand of PreviewServer tests, it makes network calls and has no reasonable path to being reenabled.

[patshaughnessy]

One idea here might be:

• Leave this code in the project, and uncomment it
• Do not include it in the normal CI test suite.
• Add a shell script that runs this integration test manually outside of the normal CI test suite.

It looks like a lot of work went into this test: the HTTPClient, etc., and might be the only place we actually test the PreviewServer class. There's limited value to keeping a test we don't normally run. But having this test code and a script that runs it might be useful in the future if we ever do change PreviewServer, for example if the underlying dependencies like Swift-NIO are updated and we want to be sure it's still working.

[d-ronnqvist]

One idea here might be:

• Leave this code in the project, and uncomment it
• Do not include it in the normal CI test suite.
• Add a shell script that runs this integration test manually outside of the normal CI test suite.

Those are all good ideas for how we could setup a suite of integration tests. We should put those ideas in an issue tracker as information for the future.

Still, it doesn't change that these tests aren't running anywhere and haven't run for the last 2.5 years.

It looks like a lot of work went into this test: the HTTPClient, etc., and might be the only place we actually test the PreviewServer class. There's limited value to keeping a test we don't normally run.

• PreviewActionIntegrationTests.testCancelsConversion() covers starting and stopping the server, so I don't think PreviewServerTests.testPreviewServerBeforeStarted() verifies anything that isn't already covered.
• FileRequestHandlerTests.testFileHandlerAssets() covers the response types for various file requests, so I don't think that PreviewServerTests.testPreviewServerPaths() verifies anything significant that isn't already covered.
• PreviewHTTPHandlerTests, DefaultRequestHandlerTests, ErrorRequestHandlerTests, and FileRequestHandlerTests in general cover the preview server's request handling.
• We could move PreviewServerTests.testPreviewServerBindDescription() to another file but the only place we use this is when printing "Stopped preview server at localhost:8080" when the user exits the preview command, so it's a very minor bit of functionality.
• The only test where I don't know if we have coverage elsewhere is PreviewServerTests.testConcurrentRequests() but I don't know if that's well suited as a unit test. We've disabled TopicRenderReferenceEncoderTests.skip_testTopicReferenceEncodingWithHighConcurrency() for similar reasons.

[d-ronnqvist]

Another way to phrase my thoughts on this:

I feel that "setup an integration test suite" is something that belongs in an issue tracker rather than as a collection of commented out or dead code. In my opinion that would both be a better place to gather ideas on how to accomplish this and it would increase the visibility of this work and increase the odds that we prioritize it in the future.

---

To further highlight how ineffective dead code is as an issue tracker; rdar://85046362 which is referenced above the unused PreviewServerTests isn't tracking fixing this test so that it can be re-enabled. Instead it was tracking disabling this test because it was causing issues in Swift CI due to not being isolated from other instances.

As an aside to this aside; I don't feel like information like that belongs in the source code either because the same information can be accessed in the git history if someone is curious.

[mayaepps]

Thanks for going through those tests to check their coverage elsewhere!

I feel that "setup an integration test suite" is something that belongs in an issue tracker rather than as a collection of commented out or dead code. In my opinion that would both be a better place to gather ideas on how to accomplish this and it would increase the visibility of this work and increase the odds that we prioritize it in the future.

This is a good point. It makes sense to me to use an issue tracker to gather ideas and visibility for this.

[d-ronnqvist]

I filed rdar://138178779 to track setting up a suite of integration tests and/or stress tests

[d-ronnqvist]

@swift-ci please test

[d-ronnqvist]

@swift-ci please test

[d-ronnqvist]

After fixing a handful of other test flakiness issues in #1070 I added back a reimplementation of PreviewActionIntegrationTests/testWatchRecoversAfterConversionErrors() in 13359b4