-
Notifications
You must be signed in to change notification settings - Fork 2.5k
SwiftTesting: Include metadata for tags, bug, and time limit traits in event stream #3040
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
base: main
Are you sure you want to change the base?
Conversation
ad4afed to
bf363fb
Compare
proposals/testing/NNNN-include-tags-bugs-and-timeline-in-event-stream.md
Outdated
Show resolved
Hide resolved
8a7e8b1 to
e7325c8
Compare
55d172e to
f56f077
Compare
| +<bug> ::= { | ||
| + ["url": <string>,] ; the bug url | ||
| + ["id": <string>,] ; the bug id | ||
| + "title": <string> ; the human readable bug title |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| + "title": <string> ; the human readable bug title | |
| + ["title": <string>] ; the human readable bug title |
The title field is not required.
| + "title": <string> ; the human readable bug title | ||
| +} ; | ||
| + | ||
| +<characters> ::= "" | <characters> <character> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to spell these out. Please remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
|
|
||
| <test-id> ::= <string> ; an opaque string representing the test case | ||
| + | ||
| +<tag> ::= "." <chacters> | <tag> ; a string representation of a tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is invalid syntax. What are you trying to describe here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to describe that a tag has this syntax .<charaters>[.<characters|.....]
ie: .Foo.bar.bar
But this conversation might be moot based on the output of https://github.com/swiftlang/swift-evolution/pull/3040/files#r2607028858
| } | ||
| ], | ||
| "tags": [ | ||
| ".blue", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think about whether the leading period should be encoded in the JSON. Since this schema may be used by other libraries that don't use Swift's dot syntax, my gut says they shouldn't be encoded and we should just define <tag> ::= <string>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think about whether the leading period should be encoded in the JSON
I don't have a strong opinion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree w/ @grynspan and think we shouldn't necessarily specify in the schema itself that there will always be a leading period character. But I do think the proposal should clearly specify whether, in the encoded string, the leading period and any subsequent punctuation will be preserved as it was written in source. The main thing I'm trying to convey is that we ought to clearly define the behavior, whatever it is.
And on this specific point, I would suggest you define the behavior such that it will precisely preserve the way the tag was spelled in source. So if the code had .tags(.blue) the string would be ".blue". If it had .tags(Tag.red), the string would be "Tag.red", and so on. The latter example differs from the example you show in the current proposal, so that represents a change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be the wrong abstraction though, as it would mean Tag.red and .red don't compare equal. (I don't think that string is available at runtime anyway!) Furthermore, preserving the source would prevent the use of this feature with libraries that define tags using different syntax. For example, C++ might have something using ::, or Objective-C might just use @"red".
I'd like to just define it formally as <string> and document that Swift Testing specifically normalizes to "red" since that would be most compatible with what other libraries or tagging syntaxes would reasonably do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to that. To be comprehensive about this, how would examples which include interstitial punctuation, like .tags(.NestedType.bar, .NestedType.AnotherNestedType.baz) be encoded in the JSON? As "NestedType.bar" and "NestedType.AnotherNestedType.baz", respectively, or something else?
In effect, this will mean we'll be treating the optional (Testing.)Tag type namespace prefixes as "special", and stripping them when present, along with the leading period. Which is fine, but it should be spelled out in the proposal. (Maybe it would be helpful to include a Markdown table with several of these examples.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already strip "Testing.Tag." when ingesting the tag at compile time, and we don't show it in the Xcode UI. Specifically for Swift Testing, I'd say the tag .foo.bar should be rendered as it is in Xcode, i.e. as "foo.bar". I recognize a hypothetical C++ library might have trouble with that spelling if they're trying to be source-accurate.
On the other hand, AFAIK one-level tag names (.red) are overwhelmingly more common in practice, so that could just be a corner case.
| +} ; | ||
| + | ||
| +<characters> ::= "" | <characters> <character> | ||
| +<chacter> ::= <letter> | <digit> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The valid set of characters in a tag (or Swift symbol name) is not limited to ASCII alphanumeric characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aack
| ### Alternative Field Naming | ||
| - **`timeLimitInSeconds` vs `timeLimit`**: We chose the shorter `timeLimit` name for | ||
| consistency with Swift Testing's existing API, with the time unit documented in the | ||
| schema specification. The naming convention was discussed with the Testing Workgroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this blurb about renaming the time fields move to Future directions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved
f56f077 to
0a650cd
Compare
0a650cd to
7530592
Compare
7530592 to
113f1a1
Compare
This proposal adds test metadata (
tags,bugsandtimeLimit) to Swift Testing's event JSON ABI, enabling richer external tooling capabilities.Links: Implementation | Forum Pitch