Fix flaky UI test - Firefox star_labs/applab/shared_apps by fisher-alice · Pull Request #51266 · code-dot-org/code-dot-org

fisher-alice · 2023-04-11T14:33:41Z

This PR:

updates the UI tests shared_apps.featureand person_project_gallery.feature,
adds a And I wait until element...has the value... command in steps.rb

My original task was to fix the flaky Firefox star_labs/applab/shared_apps UI test. I examined this consistently flaky UI test's history over the week of April 4-11 in SauceLabs Insights. Out of 70 test runs, there were 11 failures. There were an additional 6 test errors due to a SauceLabs outage on April 6. Discounting these errors, the failure rate of this UI test is ~17%.
Interestingly, the failures occurred at 2 different places in the UI test. 4/11 failures occurred after line 34 and 7 occurred after line 87.

Taking a look at several of the videos of the failed runs in Sauce Labs, I didn't notice anything unusual about the UI at the 2 failure points.

At line 35, we test that after the 'testButton1' is clicked, the text the button contains changes from 'Click Me' to 'Clicked'. It seems like when the test is checking the contained text after the click, there isn't enough time allowed for the text to change. Thus, I modify the statement on line 35 to And I wait until element "#testButton1" contains text "Clicked".

At line 88, we test that after the keys 'GLULX' are pressed in the input box, the input box text has the value of 'GLULX'. Similar to the situation above, it seems like when the test is checking the value of the input box, there isn't enough time for the value to change. Initially, I added And I wait for 3 seconds before checking the value of the input box because because I saw examples of this command in several other UI test files.

However, I discussed with @breville this approach, and he suggested instead to use a command that waited for something specific to have changed (visibility or content). When Sauce Labs is particularly slow, 3 seconds may not be sufficient, but normally, it may be longer than needed.

Although there is a command that waits for an element's contained text to change, there isn't a command that waits until an element's value has changed.

At first, I modified the And I press keys...for element... command and added a wait_short_until that waits up to 30 seconds until the element's value string matched the string composed of the keys that were pressed. It evolved quite a bit because there were many different cases to consider - these cases are described more in detail below. Eventually, I added a new command And I wait until element...has the value.... This new command can be called in flaky tests for which the subsequent command is checking the value but the value hasn't had time to change yet.

Special cases that were considered:

As I mentioned above, the condition I set for the wait_short_until was initially the element's value string matching the key string. However, there are cases when the user presses keys for an input box that already contains text.

This occurs in the personal_projects_gallery.feature test. When I watched the Sauce Labs video of a failed test run after my update, I saw that the project title 'Old Name' was in the input box and 'New Name' was appended to the existing title.
Go to 1:21.

video.mp4

Thus, I added the line And I clear the text from element ".ui-project-rename-input" before pressing the keys for 'New Name' in personal_projects_gallery.feature.

An example of a UI test for which the text in an input box is intentionally not cleared is in scenarios.feature. In the 'Scenario: Change event works in text input and text area', the user enters the keys '123' into the input box, and then '456' without clearing the previous input. Thus, the debug console prints all 6 digits.

When press keys "456\n" is executed, the newline character is not included in the value of the element.
Special keys are used such as ":enter" and ":down" in droplet.feature when the keys pressed are for element ".ace_text-input".
There are instances when we refer to user-entered data using syntax such as'#{@temp_user_data}'.

Thanks to @breville for pairing with me on this task. It was a really valuable introduction to UI tests and the Ruby language.

Links

jira ticket

Testing story

I set up locally to run tests remotely on Sauce Labs.

Deployment strategy

Follow-up work

While updating this UI test, noticed @no_ie tags still existing in test files even though we no longer run tests on IE. Remove references to tags '@eyes_ie' and '@no_ie'.
jira ticket

Privacy

Security

Caching

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

breville · 2023-04-12T18:03:04Z

    Then element "#checkbox1" is checked
    And element "#checkbox2" is checked

+  @no_ie


Is this still needed with the below fix that adds a wait?

The comment above the step definition is that 'IE does not register the key presses in this step`. So does adding a wait resolve this issue? Seems like if we're on IE, we would just wait 30 sec and then proceed to next step.

Also, I'm new to UI tests, but do we check on IE still? I don't see IE listed at https://test-studio.code.org/ui_test/test_status_UI.html.

I see you added mentioned @no_ie tags in this past PR.

Either way, I'm happy to remove tag!

Also, I'm new to UI tests, but do we check on IE still?

Hah, I don't think I do, otherwise it would be listed in browsers.json. Looks like we removed it here.

Cool! I see Darin wrote in the PR that removed IE:

Not removing IE references from - test cases with @eyes_ie or @no_ie annotations - assertions of IE compatability in "activity-guidelines" or other documents We recommend that teams spend some time searching for any cleanup opportunities.

I'll go ahead and remove this tag - ~~should I go ahead and remove this tag and comments/references to it in other files in this PR or in a separate PR?~~
I'll also create a ticket to remove the other @no_ie and @eyes_ie tags and comments/references to them.

https://codedotorg.atlassian.net/browse/SL-786

breville · 2023-04-12T22:50:09Z

  press_keys(element, key)
+  wait_short_until do
+    element_text = element.attribute("value")
+    element_text.include? key


Are there remaining known cases in which this text might not match exactly?

There were other tests that failed on the Drone build so I surmise that there are. I can follow up with an investigation to see if other tests can be updated similar to personal_projects_gallery.feature.

https://drone.cdn-code.org/code-dot-org/code-dot-org/27907

Seems reasonable. It might be worth adding a brief comment to explain this deliberate behaviour.

breville

Curious about more recent changes.

fisher-alice · 2023-04-13T15:16:08Z

Curious about more recent changes.

Yes - I've been following up on other UI tests and pushed another change based on findings. Happy to discuss!

breville · 2023-04-13T17:46:20Z

+  only_alphanumeric_backslash = key.gsub(/[^0-9a-z \\]/i, '') == key
+  check_key_values = only_alphanumeric_backslash && selector != ".ace_text-input"
+  if check_key_values
+    wait_short_until do
+      element_text = element.attribute("value")
+      input_key = key.delete "\n"
+      element_text.include? input_key
+    end
+  end


Just to capture our discussion:

For a newcomer arriving at this code, even if a lot of comments are added, it feels like there is a lot of special-casing here, mostly related to Droplet, that will be hard to figure out. I think it would probably be better to keep "I press keys X for element Y" really simple, where it just does what it says, which is press some specific keys. That means going back to how it was before.

This would mean that if we want to wait until we know the text has been entered, we would add another step that does that, and modify the tests to call that additional step. The downside is that we don't get the wait everywhere "for free" without changing the tests, but the upside is that we can customise what we look for to handle specific situations, such as in Droplet.

One other possibility would be to have two different steps here: one which is the regular "I press keys X for element Y" and does wait for the result in a typical input, and another which is a "I press keys X for element Y in Droplet" and which does the special-casing unique to Droplet.

Thanks Brendan! I opted for adding a command that presses keys and then waits for the value to change appropriately. ~~But this new command will strip out the "\n" from the input string.~~ Lmk what you think.

After discussion online, I updated the step to wait until element's value changes to expected value.

breville · 2023-04-14T00:17:29Z

    And I wait until element "#divApplab > .screen > div#text_area1" is visible
    Then element "div#text_area1" has html "Line 1<div>Line 2</div><div><br></div><div>Line3</div>"
-
+     


super nit: remove unnecessary whitespace.

breville · 2023-04-14T00:19:51Z

    Then element "#testButton1" contains text "Click me"
    When I press "testButton1"
-    Then element "#testButton1" contains text "Clicked"
+    And I wait until element "#testButton1" contains text "Clicked"


Out of curiosity would step element X eventually contains text Y also have worked?

I tried this command and it works as well.

Is there a reason to use one command vs the other?

Also, there are different versions:

When I press "testButton1" Then element "#testButton1" eventually contains text "Clicked"

And I press "testButton1" And element "#testButton1" eventually contains text "Clicked"

It doesn't seem to make a difference to use when/then vs and/and. Is there a convention? Thanks!

It looks like "wait until element X contains text Y" uses jQuery, here, injected into the browser, while "element X eventually contains text Y" uses Selenium functionality and also ensures that the element includes Y but isn't an necessarily an exact match, here and here. So I guess you can pick the best one for the job, though it's probably just a quirk of history that we have two similar but different steps. Whenever we add new steps, we should take to keep things well-factored and avoid duplicate functionality.

I believe those keywords are interchangeable. Details here.

Thank you for the refs and the resource link! Definitely will take another look to make sure that the new step I'm adding is not duplicating an existing step.

I will go ahead and use "wait until element X contains text Y" because it is used four other times in this file.

Also, I looked for an existing step command which waited a given amount of time before checking the value of an element, but I did not find it. I did see 'Then I wait up to X seconds for element Y to have css property Z equal to W". But not a step that checked the value of an element within a certain time interval.

[test all browsers] add 2 wait statements

de1ff1e

fisher-alice changed the title ~~Closed Fix flaky UI test - Firefox star_labs/applab/shared_apps~~ Fix flaky UI test - Firefox star_labs/applab/shared_apps Apr 11, 2023

fisher-alice marked this pull request as ready for review April 11, 2023 15:15

fisher-alice requested review from a team and breville April 11, 2023 15:15

fisher-alice marked this pull request as draft April 11, 2023 15:27

fisher-alice added 4 commits April 11, 2023 10:53

change testButton1 command to wait for 3 sec

4ce564a

add no_ie tag for pressing keys command

587133b

change back to wait until testButton contains text

bad5f72

add wait short until in press keys command

1234784

fisher-alice marked this pull request as ready for review April 12, 2023 18:00

breville reviewed Apr 12, 2023

View reviewed changes

breville approved these changes Apr 12, 2023

View reviewed changes

fisher-alice added 3 commits April 12, 2023 14:11

remove @no_ie tag

61799e1

fix ui test - input

1a9ce99

update press keys command

1352f79

fisher-alice requested a review from breville April 12, 2023 22:46

breville reviewed Apr 12, 2023

View reviewed changes

fisher-alice added 3 commits April 12, 2023 22:05

update scenarios.feature

7483e84

remove newlines for key press command, update how to access input box

0bfbe0b

add ace text editor to special case

15b96c4

breville requested changes Apr 13, 2023

View reviewed changes

update key press only alphanumeric/space/backslash

ee06a0b

fisher-alice requested a review from breville April 13, 2023 15:16

fisher-alice added 2 commits April 13, 2023 10:59

undo change to scenarios.feature

0f83fd9

add back newline

3a2a4a0

breville reviewed Apr 13, 2023

View reviewed changes

fisher-alice added 2 commits April 13, 2023 12:53

add another press keys command - one that waits

2dac5a7

delete \n

5c4bd11

fisher-alice added 2 commits April 13, 2023 13:11

change new commands name

706cb72

update new command - just wait

c94db13

fisher-alice requested review from a team and breville April 13, 2023 18:36

breville reviewed Apr 14, 2023

View reviewed changes

remove extra whitespace

b903070

fisher-alice requested a review from breville April 14, 2023 16:03

This was referenced Apr 14, 2023

Refactor a scenario in scenarios.feature UI test #51348

Merged

Remove references to @no-ie tag #51350

Merged

breville approved these changes Apr 19, 2023

View reviewed changes

fisher-alice merged commit a3b4578 into staging Apr 19, 2023

fisher-alice deleted the alice/fix-firefox-starlabs-applab-test branch April 19, 2023 20:42

		And I wait until element "#divApplab > .screen > div#text_area1" is visible
		Then element "div#text_area1" has html "Line 1<div>Line 2</div><div><br></div><div>Line3</div>"

Uh oh!

Conversation

fisher-alice commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisher-alice Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisher-alice Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breville left a comment

Choose a reason for hiding this comment

Uh oh!

fisher-alice commented Apr 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisher-alice Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breville Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breville Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fisher-alice commented Apr 11, 2023 •

edited

Loading

fisher-alice Apr 12, 2023 •

edited

Loading

fisher-alice Apr 12, 2023 •

edited

Loading

fisher-alice Apr 13, 2023 •

edited

Loading

breville Apr 14, 2023 •

edited

Loading

breville Apr 14, 2023 •

edited

Loading