Infrastructure: Address failing macOS regression tests #3115

stalgiag · 2024-09-16T16:50:14Z

This PR adds two small utility methods: isMacOS() and translatePlatformKeys(). The latter utility takes a single Key or an array of Keys and returns the keys with any platform-specific Keys translated. For example, Keys.Control becomes Keys.Meta on macOS.

These utilities are then used in the toolbar_toolbar test to ensure that the OS-specific modifier key is used in the "select all text" chord.

This PR also adds a small delay to a disclosure_navigation_hybrid test to handle a scroll to focus event interfering with an assertion.

see #2735

WAI Preview Link (Last built on Wed, 11 Dec 2024 17:19:43 GMT).

… toolbar_toolbar

howard-e

@stalgiag great job on getting this work started!

Found 2 additional failures when running on MacOS:

disclosure_navigation_hybrid › content/patterns/disclosure/examples/disclosure-navigation-hybrid.html [data-test-id="link-aria-current"]: "aria-current" attribute on links
combobox_grid-combo › content/patterns/combobox/examples/grid-combo.html [data-test-id="popup-key-home"]: Home from focus on list puts focus on grid and moves cursor

They were also described in #2735

─

  disclosure_navigation_hybrid › content/patterns/disclosure/examples/disclosure-navigation-hybrid.html [data-test-id="link-aria-current"]: "aria-current" attribute on links

  Rejected promise returned by test. Reason:

  ElementNotInteractableError {
    remoteStacktrace: `RemoteError@chrome://remote/content/shared/RemoteError.sys.mjs:8:8␊
    WebDriverError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:193:5␊
    ElementNotInteractableError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:353:5␊
    webdriverClickElement@chrome://remote/content/marionette/interaction.sys.mjs:167:11␊
    interaction.clickElement@chrome://remote/content/marionette/interaction.sys.mjs:136:11␊
    clickElement@chrome://remote/content/marionette/actors/MarionetteCommandsChild.sys.mjs:205:29␊
    receiveMessage@chrome://remote/content/marionette/actors/MarionetteCommandsChild.sys.mjs:85:31␊
    `,
    message: 'Element <a href="#mythical-page-content"> could not be scrolled into view',
  }

  ElementNotInteractableError: Element <a href="#mythical-page-content"> could not be scrolled into view
      at Object.throwDecodedError (/Users/howard/Projects/aria/aria-practices/node_modules/selenium-webdriver/lib/error.js:521:15)
      at parseHttpResponse (/Users/howard/Projects/aria/aria-practices/node_modules/selenium-webdriver/lib/http.js:514:13)
      at Executor.execute (/Users/howard/Projects/aria/aria-practices/node_modules/selenium-webdriver/lib/http.js:446:28)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      at async thenableWebDriverProxy.execute (/Users/howard/Projects/aria/aria-practices/node_modules/selenium-webdriver/lib/webdriver.js:742:17)
      at async /Users/howard/Projects/aria/aria-practices/test/tests/disclosure_navigation_hybrid.js:85:9



  combobox_grid-combo › content/patterns/combobox/examples/grid-combo.html [data-test-id="popup-key-home"]: Home from focus on list puts focus on grid and moves cursor

  test/tests/combobox_grid-combo.js:880

   879:                                                           
   880:     t.true(                                               
   881:       await confirmCursorIndex(t, ex.comboboxSelector, 0),

  Cursor should be at index 0 after one ARROW_HOME key

  Value is not `true`:

  false

  › test/tests/combobox_grid-combo.js:880:11

  ─

I do like this approach of starting the OS specific test utilities here and targeted a specific test. It should make resolving those other 2 issues easier to handle in the future.

I'm okay with approving and pushing for this work to get merged. We should also make sure the original issue is updated with a comment so it's clear what the 2 remaining issues are.

…conversion to key chord

…losure to previous code

…id test

…test

test/tests/combobox_grid-combo.js

stalgiag · 2024-12-04T20:14:32Z

test/tests/disclosure_navigation_hybrid.js

        await links[0].click();
+        // Add a small delay here to ensure that the scroll to focus event
+        // has time to complete and doesn't interfere with the next assertion
+        await t.context.session.sleep(300);


This test was giving intermittent failures. I determined that this was due to the scroll to focus event fired by the link click interfering with the assertion (buttons[1] always failed). This small delay fixes this issue.

stalgiag · 2024-12-04T20:16:18Z

test/util/translatePlatformKey.js

+ * @param {string|string[]} keys - The key(s) to translate
+ * @returns {string[]} - The translated key(s) as a flat array ready for spreading
+ */
+function translatePlatformKey(keys) {


This function is potentially prematurely complex. I wanted to support instances when a platform key translation may be represented by chord -> single key or single key -> chord. We do not need this functionality now since we don't use any examples of this. I am open to reverting to a map if that is preferred.

It potentially is but I could see the need for this as we move to ensuring tests are platform agnostic. Perhaps providing a clear example in the JSDoc will also be helpful.

howard-e

@stalgiag thanks so much for doing so much research into what's happening here! Left some inline comments

howard-e · 2024-12-05T13:49:17Z

.github/workflows/regression.yml

    strategy:
      fail-fast: false
      matrix:
+        os: [ubuntu-latest, macos-latest]


Adding this alone won't be enough because the grep -P flag being used in regression-tests.sh is invalid for macOS's grep. MacOS uses BSD grep which doesn't support perl compatible regex. GNU grep does.

The invalid use is being thrown in the build log (although not throwing an error code).

invalid grep -P option message

> Run ./scripts/regression-tests.sh grep: invalid option -- P usage: grep [-abcdDEFGHhIiJLlMmnOopqRSsUVvwXxZz] [-A num] [-B num] [-C[num]] [-e pattern] [-f file] [--binary-files=value] [--color=when] [--context[=num]] [--directories=action] [--label] [--line-buffered] [--null] [pattern] [file ...]

A few options here could be to:

Rewriting regression-tests.sh grep commands to not use -P

Converting regression-tests.sh to use perl commands directly

Using GNU's homebrew provided grep in the workflow. (I already have an example of this here and it seems like the most straightforward option to do)

Thanks for this! I decided to go with number 3 since you already have a well-written example.

With these updates, I see that there are a handful of tests that do not fail on my local machine but do fail on macOS in the Github workflows. Unfortunately, many of these appear to be flakey failures. I see two options:

Revert the changes that add mac regression testing to CI

Address the new set of tests that only fail on CI in this PR as well

I am open to either. I just don't want to merge new workflows in that stand to impede development with flakey failures. Curious what your thoughts are.

Yep, definitely flaky. I prefer going with number 1 here. I'd rather there be some dedicated focus and surety so as to not introduce additional CI flakiness. Would rather a follow up PR to introduce these workflows after some robust testing.

Also re-ran the tests multiple times on my macOS and not seeing those failures, so some dedicated effort on figuring out why it's happening in CI would be preferred.

I agree. Reverted!

test/tests/combobox_grid-combo.js

howard-e · 2024-12-05T15:24:05Z

test/util/translatePlatformKey.js

+ * @param {string|string[]} keys - The key(s) to translate
+ * @returns {string[]} - The translated key(s) as a flat array ready for spreading
+ */
+function translatePlatformKey(keys) {


It potentially is but I could see the need for this as we move to ensuring tests are platform agnostic. Perhaps providing a clear example in the JSDoc will also be helpful.

howard-e · 2024-12-05T15:33:21Z

test/tests/toolbar_toolbar.js

@@ -1115,7 +1116,8 @@ ariaTest(
  'toolbar-button-enter-or-space',
  async (t) => {
    let textarea = await t.context.session.findElement(By.css('textarea'));
-    await textarea.sendKeys(Key.chord(Key.CONTROL, 'a'));
+    let modifierKey = translatePlatformKey(Key.CONTROL);


This being modifierKey as an array could be confusing. Renaming to modifierKeys works? Although maybe just as confusing given a single value was passed. I suppose this goes back to having the function provide a clear example of usage

Good idea! I addressed with aa905f6.

Co-authored-by: Howard Edwards <howard-e@users.noreply.github.com>

…ia-practices into fix-macos-regression-tests

howard-e

@stalgiag thanks for addressing the feedback. These changes LGTM now and should also make future instances of platform based issues easier to handle!

stalgiag added 2 commits September 16, 2024 09:39

Use Key.META for text area all text selection for OS agnostic testing…

efaa27c

… toolbar_toolbar

Use getOSPreferredModifierKey() utility to off cross-platform support

25084fd

daniel-montalvo mentioned this pull request Sep 16, 2024

apg/fix-toolbar_toolbar-tests generated by aria-practices w3c/wai-aria-practices#352

Closed

stalgiag changed the title ~~Use Key.META for text area all text selection for OS agnostic testing in toolbar_toolbar~~ Use OS-specific modifier key for select all text in toolbar_toolbar test Sep 16, 2024

stalgiag changed the title ~~Use OS-specific modifier key for select all text in toolbar_toolbar test~~ Use OS-specific modifier key for 'select all text' command in toolbar_toolbar test Sep 16, 2024

howard-e approved these changes Nov 20, 2024

View reviewed changes

stalgiag added 4 commits November 21, 2024 18:07

Fix disclosure_navigation_hybrid test, support platform specific key …

ef0f7ba

…conversion to key chord

Skip tests in combobox that cannot be replicated on macOS, reset disc…

0a84f39

…losure to previous code

Remove errant Home and End assumption in combobox test

75bf788

Example click retry logic for flakey macOS disclosure_navigation_hybr…

900f783

…id test

stalgiag marked this pull request as draft December 4, 2024 19:28

stalgiag added 2 commits December 4, 2024 11:46

Add small delay to handle scroll event, disclosure_navigation_hybrid …

856188e

…test

Add macOS regression test runner, don't test cursor move on macOS

05cdaa6

stalgiag marked this pull request as ready for review December 4, 2024 20:09

stalgiag changed the title ~~Use OS-specific modifier key for 'select all text' command in toolbar_toolbar test~~ Address failing macOS regression tests, add macOS regression tests to workflow suite Dec 4, 2024

stalgiag commented Dec 4, 2024

View reviewed changes

test/tests/combobox_grid-combo.js Show resolved Hide resolved

stalgiag commented Dec 4, 2024

View reviewed changes

stalgiag requested a review from howard-e December 4, 2024 20:16

Missing matrix os yaml property for macos regression testing

7b3c6a5

howard-e reviewed Dec 5, 2024

View reviewed changes

stalgiag and others added 5 commits December 5, 2024 09:39

Use GNU grep in regression tests

a0b7fde

Add GNU grep for macOS in regression-tests workflow

a4688c0

Co-authored-by: Howard Edwards <howard-e@users.noreply.github.com>

Merge branch 'fix-toolbar_toolbar-tests' of https://github.com/w3c/ar…

bc9922f

…ia-practices into fix-macos-regression-tests

Revert mac os CI testing

60d40b1

JSDoc example for translatePlatformKeys, rename translatePlatformKey

aa905f6

stalgiag mentioned this pull request Dec 11, 2024

Editable Combobox with Grid Popup Example, Home and End keys do not have expected behavior on macOS #3191

Open

Add TODO and issue link for combobox grid combo

607299d

stalgiag requested a review from howard-e December 11, 2024 17:20

howard-e changed the title ~~Address failing macOS regression tests, add macOS regression tests to workflow suite~~ Infrastructure: Address failing macOS regression tests, add macOS regression tests to workflow suite Dec 12, 2024

howard-e added Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation regression-testing Related to AVA regression tests of example pages or AVA framework implementation within repo labels Dec 12, 2024

howard-e approved these changes Dec 12, 2024

View reviewed changes

howard-e changed the title ~~Infrastructure: Address failing macOS regression tests, add macOS regression tests to workflow suite~~ Infrastructure: Address failing macOS regression tests Dec 12, 2024

howard-e merged commit 37c5fc7 into main Dec 12, 2024
17 checks passed

howard-e deleted the fix-toolbar_toolbar-tests branch December 12, 2024 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infrastructure: Address failing macOS regression tests #3115

Infrastructure: Address failing macOS regression tests #3115

stalgiag commented Sep 16, 2024 •

edited by w3cgruntbot

Loading

howard-e left a comment

stalgiag Dec 4, 2024

stalgiag Dec 4, 2024

howard-e Dec 5, 2024

howard-e left a comment

howard-e Dec 5, 2024

stalgiag Dec 5, 2024

howard-e Dec 5, 2024

stalgiag Dec 5, 2024

howard-e Dec 5, 2024

howard-e Dec 5, 2024

stalgiag Dec 11, 2024

howard-e left a comment

Infrastructure: Address failing macOS regression tests #3115

Infrastructure: Address failing macOS regression tests #3115

Conversation

stalgiag commented Sep 16, 2024 • edited by w3cgruntbot Loading

howard-e left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howard-e left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howard-e left a comment

Choose a reason for hiding this comment

stalgiag commented Sep 16, 2024 •

edited by w3cgruntbot

Loading