Propagate method async-ness during test discovery
#258
Conversation
briancroom
requested review from
abertelrud,
aciidgh,
friedbunny,
neonichu and
tomerd
as code owners
|
@swift-ci please test |
| public var name: String | ||
| public var module: String | ||
| public var methods: [String] | ||
| public var testMethods: [TestMethod] | ||
| @available(*, deprecated, message: "use testMethods instead") public var methods: [String] |
There was a problem hiding this comment.
I'm not too sure what the source compatibility requirement on this is. Let me know if this should be different!
Sources/TSCUtility/IndexStore.swift
Outdated
| @@ -14,9 +14,24 @@ import TSCBasic | |||
| public final class IndexStore { | |||
|
|
|||
| public struct TestCaseClass { | |||
| public enum TestMethod: Hashable, Comparable { | |||
There was a problem hiding this comment.
Would it be more flexible as a struct instead of an enum? Basically
public struct TestMethod: Hashable, Comparable {
var name: String
var isAsync: Bool
}The way it is currently i feel like ties its basic modeling to this async-or-not distinction, but that’s really just one detail about a test method, and I wonder if it would be more future-proof to allow it to have stored state. I bring it up in part because this type is public, thereby raising the importance of modeling it well since it may be harder to change since it’s exported outside its module.
There was a problem hiding this comment.
I agree that this looks cleaner as a struct.
Sources/TSCUtility/IndexStore.swift
Outdated
|
|
||
| func add(klass: String, method: String) { | ||
| func add(klass: String, method: TestCaseClass.TestMethod) { |
There was a problem hiding this comment.
Nitpick: klass: String -> classNamed className: String?
| public let name: String | ||
| public let isAsync: Bool | ||
|
|
||
| public static func < (lhs: IndexStore.TestCaseClass.TestMethod, rhs: IndexStore.TestCaseClass.TestMethod) -> Bool { |
There was a problem hiding this comment.
Do you need full Comparable or just Equatable? Equatable should be fully synthesized.
There was a problem hiding this comment.
Lower down, we sort these, so I think we do actually need Comparable.
briancroom
force-pushed
the
bcroom/async-tests
branch
from
5a40233 to
bfa0465
Compare
|
@swift-ci please test |
|
Thanks for the feedback! |
Extend IndexStore.TestCaseClass to include information about
asynctest methods.This adopts the indexer enhancement from swiftlang/llvm-project#3452
This should be reviewed with a corresponding SwiftPM change, to be posted, which is also where a test is included.