Support disabling reporting of uncaught exceptions in tests #3736
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The solution for #1205 may be undesirable for those who already have their own solution, like setting the default exception handlers. In this case, there's a usability issue without the corresponding benefit: instead of all tests being ran and the exceptions from them being reported, unrelated tests may fail, making looking for problems more difficult. This is probably a very rare issue, so we don't provide public API for that, instead introducing a need-to-know internal variable.
|
This isn't tested for two reasons:
|
qwwdfsad
approved these changes
May 3, 2023
|
I could, but I don't see a way to fix the issue after thinking about it. Even when the |
|
(For the record, the test I used to check that everything works; not sure if I should include it in the code, it seems like a deadweight): /**
* Tests that the uncaught exceptions that happen during the test are not reported if the feature is disabled.
*/
@Test
@Ignore // this test can only be run manually, because it will only work if it's the first test to run.
fun testDisablingStrayUncaughtExceptionReporting() {
var exception: Throwable? = null
Thread.setDefaultUncaughtExceptionHandler { t, e -> exception = e }
catchNonTestRelatedExceptions = false
runTest {
val job = GlobalScope.launch {
throw RuntimeException("x")
}
job.join()
}
assertEquals("x", exception?.message)
} |
We could start with a deprecation, so either folks will move from their workarounds (e.g. fix leaking tests) or report a descriptive use-case for us |
|
Something like this #3738? |
dkhalanskyjb
deleted the
support-not-reporting-uncaught-exceptions-in-tests
branch
|
Thanks for the solution, @dkhalanskyjb. 🙂 How should we disable it? I am trying with this, but it's not accessing the variable 
Neither this way: |
|
Could you clarify here #3738 why you want to disable the functionality? The overwhelming majority of projects should never ever need this. To answer your question: one way would be to use reflection, like Class.forName("kotlinx.coroutines.test.TestScopeKt")
.getDeclaredMethod("setCatchNonTestRelatedExceptions", Boolean::class.java)
.invoke(null, true)This only works on the JVM, though. If you need this for other platforms as well, some other specific trickery is needed. |
This was referenced Jul 24, 2023
|
@dkhalanskyjb the above solution you proposed, where to put the code I tried to add it in before block of test doesn't seems to work |
LZRS
added a commit
to opensrp/fhircore
that referenced
this pull request
Jul 30, 2023
sevenreup
added a commit
to opensrp/fhircore
that referenced
this pull request
Aug 1, 2023
* rework auth items * init tagging * update register * update * update sync * update sync key * fix location syncs * fix activity listener * test fixes * fixes * replace app with default Update ConfigurationRegistryTest.kt * tests fixed * stuff * test fixes * Update CqlContentTest.kt * add tests * add tests * update appsettings * update * update * update QuestViewmodel * Update QuestionnaireActivity.kt * Update LoginViewModel.kt * Update TokenAuthenticator.kt * update auth * Update AppSettingActivityTest.kt * updates * update resource tags * block to finish saving data * update * spotless run * Update network_security_config.xml * Fix failed AppNotIdleException for some Compose tests robolectric/robolectric#7055 robolectric/robolectric#7055 (comment) * Disable catching of non-test related exceptions Kotlin/kotlinx.coroutines#3736 (comment) * add tests * Update SharedPreferencesHelperTest.kt * Update AndroidExtensions.kt * update tests --------- Co-authored-by: L≡ZRS <12814349+LZRS@users.noreply.github.com>
sevenreup
added a commit
to opensrp/fhircore
that referenced
this pull request
Aug 2, 2023
* rework auth items * init tagging * update register * update * update sync * update sync key * fix location syncs * fix activity listener * test fixes * fixes * replace app with default Update ConfigurationRegistryTest.kt * tests fixed * stuff * test fixes * Update CqlContentTest.kt * init * add tests * add tests * save * update appsettings * data clerks can log in * format * update * update * add sync status * update ui * update QuestViewmodel * Update AppScreen.kt * Update QuestionnaireActivity.kt * update update * Update LoginViewModel.kt * update sync status * update sync * Update TokenAuthenticator.kt * update icon * format * update auth * Update AppSettingActivityTest.kt * updates * update create button * Update DataClerkConfigService.kt * update resource tags * load patients * update data clerk * update patient details * update details * add pagination * add sync indicators to ui * reload on sync on the list * update * Update build.gradle * update * block to finish saving data * update * spotless run * Update network_security_config.xml * Update network_security_config.xml * Fix failed AppNotIdleException for some Compose tests robolectric/robolectric#7055 robolectric/robolectric#7055 (comment) * Disable catching of non-test related exceptions Kotlin/kotlinx.coroutines#3736 (comment) * add tests * Update SharedPreferencesHelperTest.kt * Update AndroidExtensions.kt * update tests * fix merge issues * add last updated * update network * fix pagination issues * add prod config --------- Co-authored-by: L≡ZRS <12814349+LZRS@users.noreply.github.com>
samiuelson
added a commit
to woocommerce/woocommerce-android
that referenced
this pull request
Nov 16, 2023
This is a hacky workaround for the change in kotlinx.coroutines 1.7.0 that causes tests that throw exceptions to fail. Previously test methods that threw exceptions would not prevent tests from passing, which was a bug in kotlinx.coroutines that has now been fixed. However, significant number of our tests are currently failing because of this change, because we have bugs in the implementation of our test classes, usually related to the way we instantiate the view models in tests. See the following issue for more details: Kotlin/kotlinx.coroutines#1205. The workaround coming with this commit is taken from the related PR: Kotlin/kotlinx.coroutines#3736 and is a solution suggested by JetBrains to disable the new behavior using non-public API until we fix our tests. This should not be considered a long-term solution, rather a temporary hack.
samiuelson
added a commit
to woocommerce/woocommerce-android
that referenced
this pull request
Nov 16, 2023
This is a hacky workaround for the change in kotlinx.coroutines 1.7.0 that causes tests that throw exceptions to fail. Previously test methods that threw exceptions would not prevent tests from passing, which was a bug in kotlinx.coroutines that has now been fixed. However, significant number of our tests are currently failing because of this change, because we have bugs in the implementation of our test classes, usually related to the way we instantiate the view models in tests. See the following issue for more details: Kotlin/kotlinx.coroutines#1205. The workaround coming with this commit is taken from the related PR: Kotlin/kotlinx.coroutines#3736 and is a solution suggested by JetBrains to disable the new behavior using non-public API until we fix our tests. This should not be considered a long-term solution, rather a temporary hack.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The solution for #1205 may be undesirable for those who already have their own solution, like setting the default exception handlers. In this case, there's a usability issue without the corresponding benefit: instead of all tests being ran and the exceptions from them being reported, unrelated tests may fail, making looking for problems more difficult.
This is probably a very rare issue, so we don't provide public API for that, instead introducing a need-to-know internal variable.