Skip to content

Third time's the charm #3753

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

Merged
merged 1 commit into from
May 12, 2023
Merged

Conversation

dkhalanskyjb
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb commented May 12, 2023

Fixes #3673
Thanks, @ephemient!

Context: #3742 (comment)

@dkhalanskyjb dkhalanskyjb requested a review from qwwdfsad May 12, 2023 11:51
@dkhalanskyjb
Copy link
Collaborator Author

Actually, this isn't required. If we look at the sources of how this is used in the problematic library, the dispatch timeout is not used at all, runTest is only called as a workaround when no coroutines are even left to run. Still, it's better to be safe than sorry.

Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Please add a rationale and short explanation to the squashed commit message

@dkhalanskyjb dkhalanskyjb merged commit e86dc85 into develop May 12, 2023
@dkhalanskyjb dkhalanskyjb deleted the dk-fix-runTest-compatibility-again branch May 12, 2023 12:17
@@ -580,6 +580,6 @@ internal expect fun dumpCoroutines()
public fun TestScope.runTestLegacy(
dispatchTimeoutMs: Long,
testBody: suspend TestScope.() -> Unit,
unused1: Int,
marker: Int,
unused2: Any?,

Choose a reason for hiding this comment

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

Maybe rename the parameters to mask: Int, marker: Any?.

Choose a reason for hiding this comment

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

Ah, I'm too slow. Well it shouldn't really matter.

Copy link
Collaborator Author

@dkhalanskyjb dkhalanskyjb May 12, 2023

Choose a reason for hiding this comment

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

I disagree. unused2 lends this function some tongue-in-a-cheek charm, which it deserves.

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