Skip to content

Support parameterized swift-testing tests #797

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

Conversation

plemarquand
Copy link
Contributor

Swift-testing emits a test event at the beginning of a test run that enumerates all the parameterized test cases to be executed.

This patch waits for the test event and then generates vscode.TestItems for each parameterized test execution, parenting them to their test. Then when we recieve a runStarted event we enqueue all the tests along with the newly created parameterized TestItems.

Before a new test run starts we remove the existing parameterized TestItems so we can regenerate them, as there may be a different number of parameterized tests run with every execution.

@plemarquand plemarquand force-pushed the parameterized-swift-testing-test-results-2 branch from e2e0e36 to 4655951 Compare May 13, 2024 13:03
@adam-fowler
Copy link
Contributor

This doesn't appear to work. SKLSP is returning parameterized tests as funcName(_ :), while swift-testing is expecting funcName(_:) which doesn't include the space.

@plemarquand
Copy link
Contributor Author

@adam-fowler sorry I forgot to mention this depends on swiftlang/sourcekit-lsp#1209. For now to test you'd need to build your own sourcekit-lsp with that change included, as it looks like the nightly toolchain builds aren't working.

@adam-fowler
Copy link
Contributor

Outside of the fact I can't compile, other issues include I cannot run a test suite that includes a parameterised test, and deleting tests doesn't always delete the test in the test explorer.

Copy link
Contributor

@award999 award999 left a comment

Choose a reason for hiding this comment

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

looking good to me

Swift-testing emits a `test` event at the beginning of a test run that
enumerates all the parameterized test cases to be executed.

This patch waits for the `test` event and then generates
`vscode.TestItem`s for each parameterized test execution, parenting them
to their test. Then when we recieve a `runStarted` event we enqueue all
the tests along with the newly created parameterized `TestItem`s.

Before a new test run starts we remove the existing parameterized
`TestItems` so we can regenerate them, as there may be a different
number of parameterized tests run with every execution.
@plemarquand plemarquand force-pushed the parameterized-swift-testing-test-results-2 branch from 4655951 to 046eb22 Compare May 14, 2024 16:55
@plemarquand plemarquand requested a review from award999 May 14, 2024 16:56
Copy link
Contributor

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

The code all looks good. Couple of minor code nits
Functionality wise:

  • If you delete a parameterised test it doesn't delete it from the test explorer
  • Tests with named parameters don't run. Looks like SKLSP is giving you the wrong name for the test eg
    @Test("TestIt", arguments: 2...10) func testName(value: Int) async throws {
        #expect(value < 9)
    }
  • Parameterized test items don't have a location
  • If I ask one of them to run nothing happens. Ideally it would only run the test with that one case. If you can't do that I'm not sure if it is worth it keeping a test item for every case.

@plemarquand
Copy link
Contributor Author

If you delete a parameterised test it doesn't delete it from the test explorer

Fixed, the code to prune the test item tree works bottom up and assumed items being pruned would have no children. Added a check to still prune if all children are parameterized test results.

Tests with named parameters don't run. Looks like SKLSP is giving you the wrong name for the test eg

I couldn't reproduce this, trying on both macOS and in the Linux dev container. What were you seeing? The tests would run/complete but you wouldn't get new parameterized test results in the Test Explorer?

Parameterized test items don't have a location

This is by design, since results don't really have a "location" per se. The parent TestItem should always be visible in the Test Explorer even scrolling a long list of items, and the tests location remains available off that. Its also a handy way to distinguish if an item in the TestItem hierarchy is a parameterized test result entry or not.

If I ask one of them to run nothing happens. Ideally it would only run the test with that one case. If you can't do that I'm not sure if it is worth it keeping a test item for every case.

It isn't possible today, but we can look at it for a subsequent PR.

@adam-fowler
Copy link
Contributor

I couldn't reproduce this, trying on both macOS and in the Linux dev container. What were you seeing? The tests would run/complete but you wouldn't get new parameterized test results in the Test Explorer?

Ok. I think this was a symptom of the first issue. I had a function where the parameter didn't have a label and I edited it to include a label for the parameter, but it never deleted the test items and I was still running with those old test items. Your fix for the deletion of test items for parametized test functions fixes this also.

@adam-fowler
Copy link
Contributor

If I ask one of them to run nothing happens. Ideally it would only run the test with that one case. If you can't do that I'm not sure if it is worth it keeping a test item for every case.

It isn't possible today, but we can look at it for a subsequent PR.

Can we at least get it to run the test the parameter belongs to

@plemarquand
Copy link
Contributor Author

If I ask one of them to run nothing happens. Ideally it would only run the test with that one case. If you can't do that I'm not sure if it is worth it keeping a test item for every case.

It isn't possible today, but we can look at it for a subsequent PR.

Can we at least get it to run the test the parameter belongs to

I can, but I worry having the behaviour change in a subsequent release would be confusing. In the future it would go from running all the test parameters to running the test with just one.

Tests with parameter types that aren't Codable wouldn't be able to be run individually, and so they'd lose the play button (or rerun the test with all parameters, but I think that'd be even more confusing).

@plemarquand
Copy link
Contributor Author

@adam-fowler you were right that it didn't make sense for these parameterized test items to have run buttons at all. I've added a "runnable" tag to all the test items that can be run, associated that with the run profiles, and this omits the button on parameterized tests as you'd expect.

Screen.Recording.2024-05-16.at.2.01.15.PM.mov

@plemarquand plemarquand requested a review from adam-fowler May 16, 2024 18:12
It is possible for a test item to report children size 0
if the test run starts before children are populated
Copy link
Contributor

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

We are good here

@plemarquand plemarquand merged commit 585cb59 into swiftlang:main May 17, 2024
6 checks passed
@plemarquand plemarquand deleted the parameterized-swift-testing-test-results-2 branch May 17, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants