Skip to content

Conversation

aSemy
Copy link
Contributor

@aSemy aSemy commented Jun 15, 2024

TimeMarkCompat was only necessary because of a breaking change in Kotlin 1.7 - but that was 2 years ago and Kotest now requires more recent versions, so TimeMarkCompat can now be removed. Slack discussion.

timeInMillis() is only used for measuring times, so it can be replaced with stdlib measureTime {}/measureTimedValue {}

@aSemy aSemy force-pushed the adam/feat/remove-timesource-compat branch from 7154db3 to 6ad1a78 Compare June 15, 2024 09:39
TimeMarkCompat was only necessary because of a breaking change in Kotlin 1.7 - but that was 2 years ago and Kotest now requires more recent versions, so TimeMarkCompat can now be removed.

timeInMillis is only used for measuring times, so it can be replaced with stdlib `measureTime {}`/`measureTimedValue {}`
@aSemy aSemy force-pushed the adam/feat/remove-timesource-compat branch from 6ad1a78 to 13ddc72 Compare June 15, 2024 09:40
@sksamuel
Copy link
Member

nice update

@aSemy aSemy marked this pull request as ready for review June 15, 2024 20:40
@aSemy aSemy marked this pull request as draft June 15, 2024 20:40
try {
result = f()
listener?.invoke(ContinuallyState(result, start, end, times))
listener?.invoke(ContinuallyState(result, 0L, duration.inWholeMilliseconds, times))
Copy link
Contributor Author

@aSemy aSemy Jun 15, 2024

Choose a reason for hiding this comment

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

Just to highlight, there's a potentially breaking change here.

The start and end of a ContinuallyState will no longer reflect the system time, but start will always be 0, and end will always be the configured duration.

I think this change is acceptable because it's a minor change, and this code has been deprecated for ~3 years.

Copy link
Member

Choose a reason for hiding this comment

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

agreed, all deprecated functions are coming out for 6.0

@aSemy aSemy marked this pull request as ready for review June 15, 2024 20:55
@aSemy aSemy requested review from sksamuel and Kantis June 15, 2024 20:55
@OliverO2 OliverO2 mentioned this pull request Jun 16, 2024
20 tasks
# Conflicts:
#	kotest-assertions/kotest-assertions-core/src/jvmTest/kotlin/io/kotest/assertions/nondeterministic/EventuallyTest.kt
#	kotest-assertions/kotest-assertions-shared/src/jvmTest/kotlin/com/sksamuel/kotest/assertions/until/UntilTest.kt
@aSemy aSemy requested a review from OliverO2 June 16, 2024 21:42
Copy link
Contributor

@OliverO2 OliverO2 left a comment

Choose a reason for hiding this comment

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

Excellent!

@aSemy aSemy merged commit 773f188 into master Jun 17, 2024
@aSemy aSemy deleted the adam/feat/remove-timesource-compat branch June 17, 2024 14:06
aSemy added a commit that referenced this pull request Jun 17, 2024
Use a test TimeSource in `continually` tests.

- The test TimeSource uses a virtual time, so the tests run faster.
- The elapsed duration is reproducible, so the tests can assert the
specific elapsed time.
- Use CoroutineContext to store & retrieve the TimeSource (This helps
with testing both `continually(duration)` and `continually(config)`
functions, and doesn't require adding an option in
`ContinuallyConfiguration`)


### Dependencies

* #4105
aSemy added a commit that referenced this pull request Jun 21, 2024
Use a test TimeSource in `eventually` tests.

- The test TimeSource uses a virtual time, so the tests run faster.
- The elapsed duration is reproducible, so the tests can assert the
specific elapsed time.
- Use CoroutineContext to store & retrieve the TimeSource (This helps
with testing both `eventually(duration)` and `eventually(config)`
functions, and doesn't require adding an option in
`EventuallyConfiguration`)


Depends on: #4105 

Related: #4109

---------

Co-authored-by: OliverO2 <Oliver.O456i@gmail.com>
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