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

Prepare for 2023.3: Move tests using MyMockApplication to unittests #5421

Merged

Conversation

tpasternak
Copy link
Collaborator

If I understand this correctly, the problem existed from the beginning, it has been revealed by this change:
JetBrains/intellij-community@c973850#diff-4497b4c4afdd25574053db8b6de01a43669c1b3200a5d4d31d14da002614e400L956

In the old IntelliJ versions, if I understand correctly, the tests were successful because

  1. Some of the intergration tests were spawning the original non-mock application
  2. The non-mock application was scheduling the DaemonCodeAnalyzerImpl's runUpdate
  3. The non-mock test has finished
  4. Then the mock-based tests has started
  5. A mock-based started and installed MyMockApplication to ApplicationManager
  6. The DaemonCodeAnalyzerImpl's job was still running and it scheduled ApplicationManager.getInstance().invokeLater()
  7. As MyMockApplication was already installed, it handled the invokeLater call
  8. DaemonCodeAnalyzerImpl's runUpdate called ApplicationManager.getApplication().assertIsDispatchThread()
  9. But in MockApplication, the assertIsDispatchThread() method is implemented as empty, so nothing was crashing

Since the commit c973850dd, the dispatch thread assertion is not delegated to the Application any more, but it is done by ThreadingAssertions.assertEventDispatchThread, which is not implemented in MockApplicaion, hence it really checks whether the thread is EDT.

For this reason, we would like to separate all tests relying on MyMockApplication from the ones relying on the real Application. This would not solve the race fully, but at least it wouldn't lead to a case where a task scheduled by real Application is run by MockApplication

Checklist

  • I have filed an issue about this change and discussed potential changes with the maintainers.
  • I have received the approval from the maintainers to make this change.
  • This is not a stylistic, refactoring, or cleanup change.

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.

Discussion thread for this change

Issue number: <please reference the issue number or url here>

Description of this change

@github-actions github-actions bot added product: IntelliJ IntelliJ plugin awaiting-review Awaiting review from Bazel team on PRs labels Oct 3, 2023
If I understand this correctly, the problem existed from the beginning,
it has been revealed by this change:
JetBrains/intellij-community@c973850#diff-4497b4c4afdd25574053db8b6de01a43669c1b3200a5d4d31d14da002614e400L956

In the old IntelliJ versions, if I understand correctly, the tests were successful because
1. Some of the intergration tests were spawning the original non-mock application
2. The non-mock application was scheduling the DaemonCodeAnalyzerImpl's runUpdate
3. The non-mock test has finished
4. Then the mock-based tests has started
5. A mock-based started and installed MyMockApplication to ApplicationManager
6. The DaemonCodeAnalyzerImpl's job was still running and it scheduled ApplicationManager.getInstance().invokeLater()
7. As MyMockApplication was already installed, it handled the invokeLater call
8. DaemonCodeAnalyzerImpl's runUpdate called ApplicationManager.getApplication().assertIsDispatchThread()
9. But in MockApplication, the assertIsDispatchThread() method is implemented as empty, so nothing was crashing

Since the commit c973850dd, the dispatch thread assertion is not delegated to the Application any more,
but it is done by ThreadingAssertions.assertEventDispatchThread, which is not implemented in MockApplicaion,
hence it really checks whether the thread is EDT.

For this reason, we would like to separate all tests relying on MyMockApplication from the ones relying on
the real Application. This would not solve the race fully, but at least it wouldn't lead to a case where
a task scheduled by real Application is run by MockApplication
@tpasternak tpasternak force-pushed the separate-mock-tests-from-non-mock-ones branch from d51041d to 398925e Compare October 3, 2023 14:30
@tpasternak tpasternak mentioned this pull request Oct 3, 2023
3 tasks
@mai93 mai93 merged commit 2caabf0 into bazelbuild:master Oct 3, 2023
6 checks passed
@github-actions github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product: IntelliJ IntelliJ plugin
Projects
Development

Successfully merging this pull request may close these issues.

2 participants