Skip to content

Conversation

@himanshusinghs
Copy link
Contributor

@himanshusinghs himanshusinghs commented Oct 18, 2024

Description

Two things that I identified inducing flakiness in our UI tests were:

  1. ComboBox getting updated while tests were trying to use them, made the combobox collapse
  2. Tests were not able to recover from unexpected state and eventually failed

To address the first concern, we are now updating the combobox only when there is an actual change, otherwise not and that too wrapped in an invokeLater so that existing interactions are finished before performing the update.

Second concern is addressed in two steps:

  1. We do not want to interact with the comboboxes, right after they show up on screen because they subscribe to the StateFlow and eventually a state update follows which leads to the disruption in UI interaction. So we have put a short timeout of 1 second to allow for initial state subscription.
  2. The state transitions that depend on related are grouped together in retry-able block so that when the tests end up in an expected state they can retry from the start to achieve the expected state. One such example is openRunQueryPopup.

Update

There were two further problems identified for subsequent failures:

  1. Gradle project would never load for the very first start of the test run
  2. Tests tearDown will fail complaining about writes happening in write safe context

To address the first problem, we have worked up a series of UI steps to always perform for every test that will ensure that gradle project is in sync.

And for the second problem, we realised that the write unsafe error is more of a warning which can safely be ignored. But to be on the correct step here, we have wrapped all the actions now in an invokeLater with explicit modality state.

Note: This PR does not entirely remove flakes from our CI. There is still one source of problem (happening from time to time) which I haven't been able to understand completely. When we open a file, at times it will happen that the toolbar is not visible. There is no gradle sync or indexing happening in the background but still its almost like the IDE is in dumb mode. Not yet entirely sure why but I am happy to look into that as a follow up.

Checklist

Open Questions

@github-actions github-actions bot added the no release notes It's a chore and doesn't require release notes. label Oct 18, 2024
@github-actions
Copy link

github-actions bot commented Oct 18, 2024

Coverage Report

Overall Project 79.03% -0.13%
Files changed 85.47%

File Coverage
DatabaseComboBox.kt 58.9% -3.27%
DataSourceComboBox.kt 52.57% -3.08%

Comment on lines 50 to 51
eventually(1.minutes.toJavaDuration()) {
step("Selecting DataSource $title in toolbar") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
eventually(1.minutes.toJavaDuration()) {
step("Selecting DataSource $title in toolbar") {
step("Selecting DataSource $title in toolbar") {
eventually(1.minutes.toJavaDuration()) {

Because eventually will retry every a few milliseconds, until (in this case) 1 minute passes, so we will see the step in the logs for every retry.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are going to use this pattern of eventually/step a lot, maybe we want to have an eventually that receives the step name and does both things? So we encapsulate both calls and it's easier to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually in practice I found that step log for every retries were making it more clear about what is happening. I am thinking of step as very detailed and verbose explanation of what is happening in the UI. Agree that we probably should have eventually with a step block. I will work towards that if my earlier reasoning sounds good?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It sounds great, if it's possible can we add then the retry number? Like "Selecting DataSource $title: retry N" so it's clear in the logs that we are retrying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this here

Comment on lines +44 to +55
fun eventually(
description: String,
timeout: Duration = Duration.ofSeconds(1),
recovery: () -> Unit = {},
fn: (Int) -> Unit
) {
eventually(timeout, recovery) { attempt ->
step("$description, attempt=$attempt") {
fn(attempt)
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to address the suggestion about combining eventually and step descriptions.

@himanshusinghs himanshusinghs marked this pull request as ready for review October 23, 2024 12:18
@himanshusinghs himanshusinghs requested a review from kmruiz October 23, 2024 14:58
@himanshusinghs himanshusinghs merged commit 32eeb76 into main Oct 23, 2024
11 checks passed
@himanshusinghs himanshusinghs deleted the chore/INTELLIJ-107 branch October 23, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no release notes It's a chore and doesn't require release notes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants