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

Enable all CoroutineExecutorService tests in CI #1763

Open
BenHenning opened this issue Sep 2, 2020 · 3 comments
Open

Enable all CoroutineExecutorService tests in CI #1763

BenHenning opened this issue Sep 2, 2020 · 3 comments
Labels
enhancement End user-perceivable enhancements. Impact: Low Low perceived user impact (e.g. edge cases). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

BenHenning commented Sep 2, 2020

With the recent introduction of CoroutineExecutorService in #1764, tests were also added to verify that the service follows Java's ExecutorService contract correctly in a way that's interoperable with Oppia's test dispatchers. While the tests are fairly thorough, they were exceptionally difficult to build due to needing to coordinate between custom test dispatchers, Oppia's centralized test dispatchers, and real dispatchers to test blocking behaviors (where coroutines generally don't like blocking scenarios). Most tests are fairly stable, but a few are still flaky and will require substantial investigation to stabilize. This issue will stay open until all tests in this suite can pass with at least 100 runs in the CI environment (since it's slower than most developer machines, and these tests can be sensitive to CPU speeds).

Note also that this is 'nice-to-have' since we don't expect any code iteration to occur in CoroutineExecutorService after initial introduction.

@BenHenning BenHenning added Type: Improvement Priority: Nice-to-have This work item is nice to have for its milestone. labels Sep 2, 2020
@BenHenning BenHenning added this to the Backlog milestone Sep 2, 2020
@Broppia Broppia added Impact: Low Low perceived user impact (e.g. edge cases). dev_team labels Jul 29, 2022
@BenHenning BenHenning added the Z-ibt Temporary label for Ben to keep track of issues he's triaged. label Sep 16, 2022
@BenHenning BenHenning removed this from the Backlog milestone Sep 16, 2022
@seanlip seanlip added bug End user-perceivable behaviors which are not desirable. and removed issue_type_infrastructure labels Mar 28, 2023
@MohitGupta121 MohitGupta121 added the Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. label Jun 16, 2023
@seanlip seanlip added enhancement End user-perceivable enhancements. and removed Type: Improvement bug End user-perceivable behaviors which are not desirable. labels Jun 16, 2023
@seanlip seanlip removed the Priority: Nice-to-have This work item is nice to have for its milestone. label Jun 28, 2023
@BenHenning
Copy link
Member Author

We've learned a lot since this was originally filed. #3715 is actually a better approach as it has lots of downstream benefits (namely allowing us to coordinate execution with third-party Java libraries that allow for executor customization, like OkHttp, Retrofit, and Glide). Trying to make a coroutine executor service work was very difficult, especially in testing. I'd much rather abandon this approach in favor of the coordinated executors.

@BenHenning BenHenning closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2024
@github-actions github-actions bot reopened this Aug 29, 2024
Copy link

The issue is reopened because of the following unresolved TODOs:

@Ignore("Flaky test") // TODO(#1763): Remove & stabilize test.

@Ignore("Flaky test") // TODO(#1763): Remove & stabilize test.

@BenHenning
Copy link
Member Author

Ah, this issue can be closed with the removal of CoroutineExecutorService.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement End user-perceivable enhancements. Impact: Low Low perceived user impact (e.g. edge cases). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Projects
Development

No branches or pull requests

4 participants