Skip to content
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

add StartedTestRun type state #1

Merged
merged 1 commit into from
Oct 4, 2024
Merged

add StartedTestRun type state #1

merged 1 commit into from
Oct 4, 2024

Conversation

mimir-d
Copy link
Contributor

@mimir-d mimir-d commented Oct 4, 2024

  • this new type disallows users to misuse the run (eg. by emitting run artifacts when the run hasn't been started yet)
  • this also shows that there is little value in having a TestRun object around, while not having started, due to the only action possible on it is to start it. It may be beneficial in the future to only allow started objects to exist, instead of having inert objects passed around. This is a slight change in usage, but not very consequential.
  • also fix macros tests since this new StartedTestRun pattern highlighted that they were emitting invalid messages

- this new type disallows users to misuse the run (eg. by emitting run
  artifacts when the run hasn't been started yet)
- this also shows that there is little value in having a TestRun object
  around, while not having started, due to the only action possible on
  it is to start it. It may be beneficial in the future to only allow
  started objects to exist, instead of having inert objects passed
  around. This is a slight change in usage, but not very consequential.
- also fix macros tests since this new StartedTestRun pattern
  highlighted that they were emitting invalid messages

Signed-off-by: mimir-d <[email protected]>
Copy link

codecov bot commented Oct 4, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@mimir-d mimir-d requested a review from njordr October 4, 2024 12:56
@mimir-d
Copy link
Contributor Author

mimir-d commented Oct 4, 2024

this PR only covers the TestRun, but the pattern should be replicated for all contextual objects (eg. TestStep, MeasurementSeries).

on commenting out the StartedTestRun.scope method

the binding of the returned future lifetime captures to self: &TestRun is, in retrospect, too constraining. That closure should really hold for any for<'a>, but current rust syntax is unable to capture lifetimes in RPIT impl Future in closures. The current solution for this is BoxFuture, but that implies a stack allocation (which may not be a severe consequence in this usecase, but it does make the usage more complicated because the async blocks need to be boxed).

The other option I've considered was to remove the scope method completely and use a Drop impl to output the end message. That however has 2 issues: it doesnt support any customization of the TestRunOutcome and also, the Drop would contain fallible func calls (because I/O). This is a no-go because of these reasons.

--
Ultimately I think the boxed future is the way to go here, in order to guarantee that the end messages are emitted (similar to Thread scope() in a way). Any typestate pattern for this would not guarantee that "end" is called.

@mimir-d mimir-d merged commit f4bff99 into dev Oct 4, 2024
12 checks passed
@mimir-d mimir-d deleted the fea/typestate_run branch October 4, 2024 15:24
mimir-d added a commit that referenced this pull request Oct 4, 2024
- similar to #1, this type pattern disallows steps from doing invalid
  operations, like logging when the step hasnt started yet
- also same reasoning as in #1 around commenting out the `scope` method
  on TestStep

Signed-off-by: mimir-d <[email protected]>
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.

2 participants