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

Discourage using unrealistic catalog inputs in tests #1106

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Nov 25, 2024

Bug/issue #, if applicable:

Summary

Since before the initial open source release of DocC, rdar://61945486 has tracked updating the tests to not use the "TestBundle" which contains a lot of inaccurate and unrealistic handcrafted data. Unfortunately, use of these test inputs have increased since then.

This PR renames those "TestBundle" to "DoNotUseInNewTests" to discourage using it in new tests and updates many of the tests that only needed some content to created dedicated minimal test inputs to reduce the the use of these unrealistic test inputs. Lastly, it simplifies the loading of inputs in many tests.

Note

This is a test-only PR.

Dependencies

None.

Testing

Nothing in particular. This isn't a user-facing change.

Checklist

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

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

@d-ronnqvist d-ronnqvist added the code cleanup Code cleanup *without* user facing changes label Nov 25, 2024
Copy link
Contributor

@franklinsch franklinsch left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -39,7 +39,7 @@ class ExternalTopicsGraphHashTests: XCTestCase {

func testNoMetricAddedIfNoExternalTopicsAreResolved() throws {
// Load bundle without using external resolvers
let (_, context) = try testBundleAndContext(named: "TestBundle")
let (_, context) = try testBundleAndContext(named: "DoNotUseInNewTests")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Any thoughts on something more explicit like LegacyBundle_DoNotUseInNewTests to make it clearer that this is a test bundle?

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 like it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 86ce80f

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 5aa2c45 into swiftlang:main Dec 5, 2024
2 checks passed
@d-ronnqvist d-ronnqvist deleted the discourage-using-unrealistic-test-bundle branch December 5, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Code cleanup *without* user facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants