lib: Add error message when showing multiple Dialogs #19595
Merged
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
This was referenced Nov 9, 2023
|
A-ha! Let's fix this bug |
martinpitt
force-pushed
the
double-dialog
branch
from
81d98a1
to
e2d7951
Compare
martinpitt
commented
Nov 9, 2023
mvollmer
requested changes
Nov 10, 2023
Close the crypto policy dialog after checking the default value. Leaving it open and clicking around on the main page is cheating and prone to race conditions, and will fail with the next commit.
With human user/browser interaction there can only ever be one Modal dialog at a time. However, our tests/CDP driver does not have that restriction, tests can happily click on main page buttons while a dialog is open. This can lead to situations where a test opens a dialog, and then opens a second one (which will overwrite `WithDialogs.Dialog.dialog` with the new component) without waiting for the first one to close. Then when the first dialog handler finally calls `.close()`, it will unexpectedly close the *second* dialog instead. These races are a pain to debug [1]. Make them obvious with an error message which shows both the old and the new dialog element. As our tests now fail on console.errors(), that will detect/prevent similar race conditions in our tests. [1] cockpit-project/cockpit-machines#1292
martinpitt
force-pushed
the
double-dialog
branch
from
e2d7951
to
9c91eb5
Compare
mvollmer
approved these changes
Nov 10, 2023
cockpituous
reviewed
Nov 10, 2023
Comment on lines
+111
to
+114
| console.error("Dialogs.show() called for", | ||
| JSON.stringify(component), | ||
| "while a dialog is already open:", | ||
| JSON.stringify(dialog)); |
There was a problem hiding this comment.
These 4 added lines are not executed by any test. Details
This was referenced Nov 16, 2023
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.
With human user/browser interaction there can only ever be one Modal dialog at a time. However, our tests/CDP driver does not have that restriction, tests can happily click on main page buttons while a dialog is open.
This can lead to situations where a test opens a dialog, and then opens a second one (which will overwrite
WithDialogs.Dialog.dialogwith the new component) without waiting for the first one to close. Then when the first dialog handler finally calls.close(), it will unexpectedly close the second dialog instead.These races are a pain to debug [1]. Make them obvious with an error message which shows both the old and the new dialog element. As our tests now fail on console.errors(), that will detect/prevent similar race conditions in our tests.
[1] cockpit-project/cockpit-machines#1292