Skip to content

[test only] Combine many of the various SGF test helpers #955

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 8 commits into from
Aug 20, 2024

Conversation

d-ronnqvist
Copy link
Contributor

Bug/issue #, if applicable:

Summary

This is a test-only change that combined many of the test helpers for creating symbol graph files and symbols.

Dependencies

None

Testing

This is a test-only 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 Jun 17, 2024
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@anferbui anferbui left a comment

Choose a reason for hiding this comment

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

Minor comments on the defaults for source locations.

pathComponents: [String],
docComment: String? = nil,
accessLevel: SymbolGraph.Symbol.AccessControl = .init(rawValue: "public"), // Defined internally in SwiftDocC
location: (position: SymbolGraph.LineList.SourceRange.Position, url: URL)? = (defaultSymbolPosition, defaultSymbolURL),
Copy link
Contributor

Choose a reason for hiding this comment

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

We're duplicating these defaults 3 times in this file, it would be nice if we only used them once. I think here we could make it a non-optional value and save us needing to have defaults later on:
https://github.com/apple/swift-docc/blob/64ec387cab27ce898f74df36eff2d0d548b77658/Sources/SwiftDocCTestUtilities/SymbolGraphCreation.swift#L108-L109

Suggested change
location: (position: SymbolGraph.LineList.SourceRange.Position, url: URL)? = (defaultSymbolPosition, defaultSymbolURL),
location: (position: SymbolGraph.LineList.SourceRange.Position, url: URL) = (defaultSymbolPosition, defaultSymbolURL),

and then we won't need the defaults:

makeLineList(
    docComment: $0,
    startOffset: location.position,
    url: location.url
)

Copy link
Contributor Author

@d-ronnqvist d-ronnqvist Jul 15, 2024

Choose a reason for hiding this comment

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

There are some cases where real world data doesn't have a location, for example if the symbol graph was extracted from a binary instead of from source code.

I prefer to keep the ability to create such a value. The idea is that makeLineList, makeMixins, makeMetadata, makeModule, etc. all should be usable from any test. That's why each of them duplicate the default values so that the caller doesn't have to.

Comment on lines 571 to +572
private let start = SymbolGraph.LineList.SourceRange.Position(line: 7, character: 6) // an arbitrary non-zero start position
private let symbolURL = URL(fileURLWithPath: "/path/to/SomeFile.swift")
private let symbolURL = URL(fileURLWithPath: "/path/to/SomeFile.swift")
Copy link
Contributor

@anferbui anferbui Jul 5, 2024

Choose a reason for hiding this comment

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

Does this still need to be explicitly defined for these tests or can we use makeSymbolGraph's defaults? I ask because this is very close to the default location values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reason for this change is that by making the default values private variables, any test that wants to inspect and make assertions about these values would need to define customize the location and URL. If the test only needs a realistic value it can use the defaults. If the test has expectations about a specific value, it should define that value.

With this reasoning; it's intentional that this file uses a different URL than the default value. I'll update the default location as well so that it's not the same as this customized value.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

# Conflicts:
#	Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift
Also, make single-use helper function private
# Conflicts:
#	Sources/SwiftDocC/Model/Rendering/RenderSectionTranslator/PlistDetailsSectionTranslator.swift
#	Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@anferbui anferbui left a comment

Choose a reason for hiding this comment

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

LGTM

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 610d732 into swiftlang:main Aug 20, 2024
2 checks passed
@d-ronnqvist d-ronnqvist deleted the unify-symbol-test-helpers branch August 20, 2024 12:54
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