Add line and column to test item id

[kimdv]

Fixes #1152

[ahoppen]

@grynspan Had a very interesting idea just now to reduce possible issues where the test index still has an old location for the test but it has been moved: We only need the file-line-column distinguisher if the test ID is otherwise ambiguous, eg. because you have two @Test fileprivate func testSomething() {} functions in different files.

What we could do is:

• Change AnnotatedTestItem to essentially be a TestItem just with additional information. Differences to TestItem are
  • Has isExtension property
  • Children are also AnnotatedTestItem
  • Stores a ambiguousTestDifferentiator: String of the test (I’d prefer not to re-use the location property because it makes sense that the test’s range contains the @ but the test location needs to be at Test). This would eg. be MyFile.swift:4:2.
• In SyntacticTestIndex add a new method ambigousTestIDs(). This iterates over all tests in the project and checks if there are any test IDs that are used twice
• When translating AnnotatedTestItem to TestItem, pass in the ambiguous test IDs. If the id of an AnnotatedTestItem is in that set of ambiguous IDs, we append the ambiguousTestDifferentiator to its ID.

This is a lot more involved than what I described in the original issue. Would you be interested in trying this, @kimdv? Also: Do you think our idea makes sense?

[kimdv]

Change AnnotatedTestItem to essentially be a TestItem just with additional information.

Just to be clear, does it mean I should remove AnnotatedTestItem and replace all usage with TestItem?

[ahoppen]

I meant it the other way round: When computing the tests, everything should be AnnotatedTestItem and AnnotatedTestItem would look something like the following.

public struct TestItem: ResponseType, Equatable {
  public var id: String
  public var label: String
  public var description: String?
  public var sortText: String?
  public var disabled: Bool
  public var style: String
  public var location: Location
  public var children: [AnnotatedTestItem]
  public var tags: [TestTag]
  public var isExtension: Bool
  public var ambiguousTestDifferentiator: String
}

And only when actually creating the LSP response do we convert the AnnotatedTestItems into TestItems.

[kimdv]

@ahoppen it's been a while. Catching up.

I added a commit 2e6780c is that something that you imagined? It's still WIP

[grynspan]

@grynspan Had a very interesting idea just now to reduce possible issues where the test index still has an old location for the test but it has been moved: We only need the file-line-column distinguisher if the test ID is otherwise ambiguous, eg. because you have two @Test fileprivate func testSomething() {} functions in different files.

Sorry, I just saw this now. That's basically how Swift Testing decides whether or not to include the source location in its output to swift test list. The ID will always contain the source location, but it's not necessary to include it if the function name is unambiguous because, in effect, the function name is itself a pseudo-suite for the purposes of test discovery/planning/running, so specifying a test ID without a source location component just says "run all the tests with this name" and if there's only one… you just run the one.

Does that make sense?

[ahoppen]

is that something that you imagined?

Yes, exactly. Now we just need

• Stores a ambiguousTestDifferentiator: String of the test (I’d prefer not to re-use the location property because it makes sense that the test’s range contains the @ but the test location needs to be at Test). This would eg. be MyFile.swift:4:2.
• In SyntacticTestIndex add a new method ambigousTestIDs(). This iterates over all tests in the project and checks if there are any test IDs that are used twice
• When translating AnnotatedTestItem to TestItem, pass in the ambiguous test IDs. If the id of an AnnotatedTestItem is in that set of ambiguous IDs, we append the ambiguousTestDifferentiator to its ID.

[kimdv]

@ahoppen so far so good.

Stores a ambiguousTestDifferentiator: String of the test (I’d prefer not to re-use the location property because it makes sense that the test’s range contains the @ but the test location needs to be at Test). This would eg. be MyFile.swift:4:2.

If we not can use the location how do we then get the 4:2 etc? Maybe I'm miss understadning something? 😅

[ahoppen]

I was saying that the ambiguousTestDifferentiator should be created by whoever calls the initializer of AnnotatedTestItem and that caller should decide what the differentiator is and we shouldn’t implicitly re-use AnnotatedTestItem.location for that. Does this make sense?

[plemarquand]

@kimdv I came across this issue independently while writing some sourcekit-lsp tests and I sketched out a PR. Unfortunately I didn't see your work first! Would you be open to coauthoring this if I put my tests in to this PR?

I think we could also remove the need to unwrap the testItem from the AnnotatedTestItem and just modify the TestItem's ID directly. @ahoppen I think this simplifies things quite a bit, what do you think?