-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Try once more to restore binary compatibility in runTest
#3742
Conversation
I "patched" this into my project by defining that function locally, and there are still errors:
I was able to fix that by adding: @Deprecated(
"This is for binary compatibility with the `runTest` overload that existed at some point",
level = DeprecationLevel.HIDDEN
)
@Suppress("DEPRECATION", "UNUSED_PARAMETER", "UNUSED_VARIABLE")
public fun TestScope.runTest(
dispatchTimeoutMs: Long,
testBody: suspend TestScope.() -> Unit
) {} but I can't call |
unused2: Any?, | ||
): TestResult = runTest(dispatchTimeoutMs ?: 60_000, testBody) | ||
): TestResult = runTest(dispatchTimeoutMs, testBody) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
): TestResult = runTest(dispatchTimeoutMs, testBody) | |
): TestResult = runTest(if (unused1 and 1 != 0) DEFAULT_DISPATCH_TIMEOUT_MS else dispatchTimeoutMs, testBody) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's no need: if somebody did actually provide dispatchTimeoutMs
, the $default
overload wouldn't be chosen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that the wrong way around? We're in this function because they didn't supply all the parameters, therefore we need to fill them in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes, you're right, it is the other way around. Let me think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're fully correct, by decompiling this method in 1.6.4, we get
// $FF: synthetic method
public static void runTest$default(TestScope var0, long var1, Function2 var3, int var4, Object var5) {
if ((var4 & 1) != 0) {
var1 = 60000L;
}
TestBuildersKt.runTest(var0, var1, var3);
}
Thanks for saving us from one more potential bugfix release!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job.
I haven't dived deep into the restored method's semantics though, it would be nice to manually check compose's library normally operates before the release
On it. |
I looked into two reproducers provided to us: #3673 (comment) and #3673 (comment). On both, the issue reproduces on 1.7.0, but with a locally-published version with this patch, one project starts to build successfully, while the other changes the errors from |
Won't this still be an issue because of #3742 (comment) |
@eygraber I don't understand your approach there and don't think it would work anyway, but the likely reason for |
My point was that it seems like something is also expecting this function to be present:
Note that it returns void, and has a different signature than the function that was restored in this PR. |
Follow-up to #3742. That implementation there did work around issue #3673, but did not restore full binary compatibility: the wrong value of `dispatchTimeoutMs` was passed. Given how `runTest` was used in the problematic library, it should not be a problem, but just to be safe and establish the same behavior even in the deep corner cases, we restore the original implementation fully.
Tried with 1.7.1 and it works ¯_(ツ)_/¯ |
Fixes #3673