Skip to content

Lazily evaluate test arguments only after determining their test will run, and support throwing expressions #366

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 29, 2024
Merged

Lazily evaluate test arguments only after determining their test will run, and support throwing expressions #366

merged 4 commits into from
Apr 29, 2024

Conversation

stmontgomery
Copy link
Contributor

This changes test declaration, discovery, planning, and running such that arguments to parameterized test functions are evaluated lazily, only after determining their test will run. It also adds support for throwing expressions in test arguments.

Motivation:

When defining parameterized test functions, it can be useful to call throwing or asynchronous APIs — for example, to fetch the arguments from an external source. Today, the @Test macro supports async expressions but not throws. More problematic still: the arguments of all tests get evaluated, even for tests which don't actually run (which may happen for various reasons). Evaluating the arguments of a test which won't run wastes time and resources, so we should try to avoid this, and support throws expressions too for better flexibility.

Modifications:

  • Modify the @Test macro and supporting library interfaces to surround test arguments in closures, so their evaluation can be lazy and deferred.
  • Modify how test case arguments are stored on each Test instance accordingly.
  • Modify the planning logic in Runner.Plan to defer evaluation of test cases until we determine whether each test will run.
  • Update tests.

Result:

Now, arguments to a parameterized test will only be evaluated if that test is actually going to run, and the expressions may include try and potentially throw an Error.

Checklist:

  • Code and documentation should follow the style of the Style Guide.
  • If public symbols are renamed or modified, DocC references should be updated.

Resolves #166
Resolves rdar://121531170

@stmontgomery stmontgomery added the enhancement New feature or request label Apr 19, 2024
@stmontgomery stmontgomery self-assigned this Apr 19, 2024
@stmontgomery
Copy link
Contributor Author

@swift-ci please test

@stmontgomery stmontgomery force-pushed the lazily-evaluate-arguments branch from d898046 to 7a0007c Compare April 19, 2024 19:34
@stmontgomery
Copy link
Contributor Author

@swift-ci please test

grynspan
grynspan reviewed Apr 19, 2024
View reviewed changes
grynspan
grynspan reviewed Apr 19, 2024
View reviewed changes
grynspan
grynspan reviewed Apr 19, 2024
View reviewed changes
grynspan
grynspan reviewed Apr 19, 2024
View reviewed changes
Tests/TestingTests/RunnerTests.swift
/// purpose is to validate that the arguments of tests which are not run are
/// never evaluated.
@Test(.hidden, arguments: fatalArguments())
func parameterizedWithFatalArguments(i: Int) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like an opportunity to write an exit test here!

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't kidding—why not write one?

grynspan
grynspan reviewed Apr 19, 2024
View reviewed changes
grynspan
grynspan reviewed Apr 19, 2024
View reviewed changes
grynspan
grynspan requested changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@grynspan grynspan left a comment

Choose a reason for hiding this comment

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

See comments above. I don't know why GitHub makes me type this. :)

@stmontgomery stmontgomery mentioned this pull request Apr 23, 2024
Merged
2 tasks
stmontgomery added a commit that referenced this pull request Apr 24, 2024
@stmontgomery
Change Graph's 'mapValues' APIs to pass key path in addition to value…
6676949 
… to transform closure (#372)

This is a small change which begins passing key path _in addition to_
value to Graph's `mapValue` and `compactMapValue` APIs.

### Motivation:

This was extracted out from work in #366, and is initially intended for
use in that PR but is a generally-useful enhancement.

### Checklist:

- [x] Code and documentation should follow the style of the [Style
Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md).
- [x] If public symbols are renamed or modified, DocC references should
be updated.
@stmontgomery stmontgomery force-pushed the lazily-evaluate-arguments branch from 7a0007c to abcce80 Compare April 24, 2024 01:31
@stmontgomery stmontgomery requested a review from grynspan April 25, 2024 20:14
@stmontgomery
Copy link
Contributor Author

@swift-ci please test

grynspan
grynspan reviewed Apr 25, 2024
View reviewed changes
grynspan
grynspan reviewed Apr 25, 2024
View reviewed changes
grynspan
grynspan reviewed Apr 25, 2024
View reviewed changes
grynspan
grynspan reviewed Apr 25, 2024
View reviewed changes
grynspan
grynspan reviewed Apr 25, 2024
View reviewed changes
grynspan
grynspan reviewed Apr 25, 2024
View reviewed changes
@stmontgomery
Copy link
Contributor Author

@swift-ci please test

grynspan
grynspan reviewed Apr 29, 2024
View reviewed changes
@stmontgomery stmontgomery merged commit dcce2c7 into main Apr 29, 2024
@stmontgomery stmontgomery deleted the lazily-evaluate-arguments branch April 29, 2024 23:48
@stmontgomery stmontgomery mentioned this pull request May 30, 2024
Merged
2 tasks
stmontgomery added a commit that referenced this pull request Jun 1, 2024
@stmontgomery
Ensure @Test(arguments:) macro expansion trims trivia from argument…
41cd5d6 
… expressions (#448)

This fixes a build error which can arise if the `@Test(arguments:)`
attribute applied to a parameterized test function includes trailing
trivia such as a `//`-style line comment on the end of the line. For
example:

```swift
@test(
  arguments: [...] // some comment
)
func example(...) { ... }
```

This regressed with my changes in #366 which began surrounding the
argument expressions in a closure expression, in order to facilitate
lazy evaluation. The fix ensures that the wrapped expression is trimmed,
in addition to the outer closure (which was, and still is, trimmed).

### Motivation:

Comments in this position may not be used often, but can be useful to
provide context about the role or meaning of a particular collection of
arguments passed to a parameterized test. So we'd like to continue
supporting this.

### Modifications:

- Trim trivia from the argument expression included in the `@Test` macro
expansion code.
- Add test reproducing and validating this scenario.

### Result:

Parameterized test functions with trailing line comments on the
`arguments:` line now produce valid macro expansion code and build
successfully.

### Checklist:

- [x] Code and documentation should follow the style of the [Style
Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md).
- [x] If public symbols are renamed or modified, DocC references should
be updated.

Resolves rdar://128609944
@briancroom briancroom mentioned this pull request Jun 27, 2024
Merged
2 tasks
briancroom added a commit that referenced this pull request Jun 27, 2024
@briancroom
Skip evaluating parameterized test arguments for disabled tests (#507)
aacacd7 
Similar to how parameterized test arguments are left unevaluated for
tests that aren't included in a `Plan`, also leave them unevaluated for
tests in a `Plan` that won't run, e.g. because of having the
`.disabled()` trait attached.

### Motivation:

Since the changes in #366, parameterized test arguments are evaluated
lazily, only for tests which are included in the `Runner.Plan` that is
being prepared for execution. A code comment included in that change
further suggested that arguments are only evaluated for tests which will
actually run as part of the plan, as opposed to being skipped. However
the actual code didn't reflect that behavior, by mistake. That means
wasted work is being performed when setting up a `Runner.Plan`,
evaluating the arguments of tests which won't end up actually running.

### Modifications:

Only perform the step of evaluating test arguments/cases for tests which
are marked with a `.run` action in the `Runner.Plan`.

### Result:

Less throwaway work is performed when setting up a `Runner.Plan` for
greater efficiency.

### Checklist:

- [x] Code and documentation should follow the style of the [Style
Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md).
- [x] If public symbols are renamed or modified, DocC references should
be updated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grynspan grynspan grynspan approved these changes

@dennisweissmann dennisweissmann Awaiting requested review from dennisweissmann

@briancroom briancroom Awaiting requested review from briancroom briancroom is a code owner

@SeanROlszewski SeanROlszewski Awaiting requested review from SeanROlszewski

@suzannaratcliff suzannaratcliff Awaiting requested review from suzannaratcliff suzannaratcliff is a code owner

Assignees

@stmontgomery stmontgomery

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support throwing errors from within @Test attributes
2 participants
@stmontgomery @grynspan