OSOE-530: Fixing that FeatureCodeGeneration UI test can be flaky with StaleElementReferenceException #109
Conversation
github-actions
bot
changed the title
| // so we need to retry for these random issues. | ||
| var succeeded = false; | ||
| const int maxTries = 3; | ||
| for (var i = 1; !succeeded && i <= maxTries; i++) |
There was a problem hiding this comment.
var i = 1 please use more meaningful variable name.
There was a problem hiding this comment.
Can't we use ReliabilityUITestContextExtensions.RetryIfStaleOrFailAsync instead?
There was a problem hiding this comment.
i is the index variable, like in every for loop ever. Why would we use a different name?
I'll check out RetryIfStaleOrFailAsync.
There was a problem hiding this comment.
I know, usually i j k is used as a loop variable.
We can name it like currentTry.
Here are some example for inspiration:
There was a problem hiding this comment.
I see, let's use currentTry.
I checked out RetryIfStaleOrFailAsync and due to us having maxTries it won't help.
There was a problem hiding this comment.
Could we create a reusable extension like RetryIfStaleOrFailAsync? With maxRetry instead of timeouts.
There was a problem hiding this comment.
Now that you say it, actually, we shouldn't use tries here but timeouts. For UI tests, it's more meaningful to say "try this for 10s" than to "try this 10 times" (which can take any amount of time, but for test execution we care about the runtime).
| await context.RetryIfStaleOrFailAsync(async () => | ||
| { | ||
| try | ||
| { | ||
| await context.ClickReliablyOnAsync(By.ClassName("toggle-showing-generated-migration-code")); | ||
|
|
||
| context.Get(By.Id("generated-migration-code").OfAnyVisibility()).GetValue().ShouldBe(GeneratedMigrationCodes.Page); | ||
|
|
||
| // Making sure that the collapsible area is open. | ||
| context.Get(By.CssSelector("#generated-migration-code-container.collapse.show")); | ||
|
|
||
| // Checking the first line of the CodeMirror editor. | ||
| context.Get(By.CssSelector(".CodeMirror-line .cm-variable")).Text.ShouldBe("_contentDefinitionManager"); | ||
| context.Get(By.CssSelector(".CodeMirror-line .cm-property")).Text.ShouldBe("AlterTypeDefinition"); | ||
| context.Get(By.CssSelector(".CodeMirror-line .cm-string")).Text.ShouldBe("\"Page\""); | ||
|
|
||
| return true; | ||
| } | ||
| catch (StaleElementReferenceException) | ||
| { | ||
| context.Refresh(); | ||
| throw; | ||
| } | ||
| }); |
There was a problem hiding this comment.
I would suggest since the StaleElementReferenceException is handled by RetryIfStaleOrFailAsync.
My opinion is the place for context.Refresh after context.ClickReliablyOnAsync would be better since DOM changes can occur after that call.
| await context.RetryIfStaleOrFailAsync(async () => | |
| { | |
| try | |
| { | |
| await context.ClickReliablyOnAsync(By.ClassName("toggle-showing-generated-migration-code")); | |
| context.Get(By.Id("generated-migration-code").OfAnyVisibility()).GetValue().ShouldBe(GeneratedMigrationCodes.Page); | |
| // Making sure that the collapsible area is open. | |
| context.Get(By.CssSelector("#generated-migration-code-container.collapse.show")); | |
| // Checking the first line of the CodeMirror editor. | |
| context.Get(By.CssSelector(".CodeMirror-line .cm-variable")).Text.ShouldBe("_contentDefinitionManager"); | |
| context.Get(By.CssSelector(".CodeMirror-line .cm-property")).Text.ShouldBe("AlterTypeDefinition"); | |
| context.Get(By.CssSelector(".CodeMirror-line .cm-string")).Text.ShouldBe("\"Page\""); | |
| return true; | |
| } | |
| catch (StaleElementReferenceException) | |
| { | |
| context.Refresh(); | |
| throw; | |
| } | |
| }); | |
| await context.RetryIfStaleOrFailAsync(async () => | |
| { | |
| await context.ClickReliablyOnAsync(By.ClassName("toggle-showing-generated-migration-code")); | |
| context.Refresh(); | |
| context.Get(By.Id("generated-migration-code").OfAnyVisibility()).GetValue().ShouldBe(GeneratedMigrationCodes.Page); | |
| // Making sure that the collapsible area is open. | |
| context.Get(By.CssSelector("#generated-migration-code-container.collapse.show")); | |
| // Checking the first line of the CodeMirror editor. | |
| context.Get(By.CssSelector(".CodeMirror-line .cm-variable")).Text.ShouldBe("_contentDefinitionManager"); | |
| context.Get(By.CssSelector(".CodeMirror-line .cm-property")).Text.ShouldBe("AlterTypeDefinition"); | |
| context.Get(By.CssSelector(".CodeMirror-line .cm-string")).Text.ShouldBe("\"Page\""); | |
| return true; | |
| }); |
There was a problem hiding this comment.
Opening the code block doesn't retain its state between page loads (I don't see why it would need to), so a refresh in between won't work.
There was a problem hiding this comment.
Yes, sorry, I mixed it with an other context.Refresh
| await context.RetryIfStaleOrFailAsync(async () => | ||
| { | ||
| try | ||
| { | ||
| await context.ClickReliablyOnAsync(By.ClassName("toggle-showing-generated-migration-code")); | ||
|
|
||
| context.Get(By.Id("generated-migration-code").OfAnyVisibility()).GetValue().ShouldBe(GeneratedMigrationCodes.Page); | ||
|
|
||
| // Making sure that the collapsible area is open. | ||
| context.Get(By.CssSelector("#generated-migration-code-container.collapse.show")); | ||
|
|
||
| // Checking the first line of the CodeMirror editor. | ||
| context.Get(By.CssSelector(".CodeMirror-line .cm-variable")).Text.ShouldBe("_contentDefinitionManager"); | ||
| context.Get(By.CssSelector(".CodeMirror-line .cm-property")).Text.ShouldBe("AlterTypeDefinition"); | ||
| context.Get(By.CssSelector(".CodeMirror-line .cm-string")).Text.ShouldBe("\"Page\""); | ||
|
|
||
| return true; | ||
| } | ||
| catch (StaleElementReferenceException) | ||
| { | ||
| context.Refresh(); | ||
| throw; | ||
| } | ||
| }); |
There was a problem hiding this comment.
BTW This approach does not seem good, since the click on the button toggles the generated-migration-code-container visibility. So If the first try any element gets stale, then in the next cycle, the button gets clicked again which possibly removes the show css class and the generated-migration-code-container get hidden, after this the context. Get will throw a not found exception and the test fail.
There was a problem hiding this comment.
A possible solution is to move out await context.ClickReliablyOnAsync(By.ClassName("toggle-showing-generated-migration-code")); of the cycle, then it will be called only once while the other checks retried in case of error.
There was a problem hiding this comment.
The click needs to be done every time to open the code area, see my comment above #109 (comment). So it needs to be repeated after a refresh. The refresh in turn is needed so if anything is stale (what it really shouldn't), it gets resolved.
This works in a loop, I checked.
There was a problem hiding this comment.
Yes, sorry, I mixed it with an other context.Refresh
OSOE-530
Fixes #108.