Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infrastructure: Address failing macOS regression tests #3115

Merged
merged 15 commits into from
Dec 12, 2024
Merged

Infrastructure: Address failing macOS regression tests #3115

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
efaa27c
Use Key.META for text area all text selection for OS agnostic testing…
stalgiag Sep 16, 2024
25084fd
Use getOSPreferredModifierKey() utility to off cross-platform support
stalgiag Sep 16, 2024
ef0f7ba
Fix disclosure_navigation_hybrid test, support platform specific key …
stalgiag Nov 22, 2024
0a84f39
Skip tests in combobox that cannot be replicated on macOS, reset disc…
stalgiag Nov 22, 2024
75bf788
Remove errant Home and End assumption in combobox test
stalgiag Dec 4, 2024
900f783
Example click retry logic for flakey macOS disclosure_navigation_hybr…
stalgiag Dec 4, 2024
856188e
Add small delay to handle scroll event, disclosure_navigation_hybrid …
stalgiag Dec 4, 2024
05cdaa6
Add macOS regression test runner, don't test cursor move on macOS
stalgiag Dec 4, 2024
7b3c6a5
Missing matrix os yaml property for macos regression testing
stalgiag Dec 4, 2024
a0b7fde
Use GNU grep in regression tests
stalgiag Dec 5, 2024
a4688c0
Add GNU grep for macOS in regression-tests workflow
stalgiag Dec 5, 2024
bc9922f
Merge branch 'fix-toolbar_toolbar-tests' of https://github.com/w3c/ar…
stalgiag Dec 5, 2024
60d40b1
Revert mac os CI testing
stalgiag Dec 5, 2024
aa905f6
JSDoc example for translatePlatformKeys, rename translatePlatformKey
stalgiag Dec 11, 2024
607299d
Add TODO and issue link for combobox grid combo
stalgiag Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/regression.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ on:

jobs:
regression:
runs-on: ubuntu-latest
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest]
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. Rewriting regression-tests.sh grep commands to not use -P
  2. Converting regression-tests.sh to use perl commands directly
  3. 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. Revert the changes that add mac regression testing to CI
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Reverted!

CI_NODE_INDEX: [0, 1, 2, 3, 4]

steps:
Expand Down
14 changes: 9 additions & 5 deletions test/tests/combobox_grid-combo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const assertAriaLabelledby = require('../util/assertAriaLabelledby');
const assertAttributeValues = require('../util/assertAttributeValues');
const assertAttributeDNE = require('../util/assertAttributeDNE');
const assertAriaRoles = require('../util/assertAriaRoles');

const isMacOS = require('../util/isMacOS');
const exampleFile = 'content/patterns/combobox/examples/grid-combo.html';

const ex = {
Expand Down Expand Up @@ -877,10 +877,14 @@ ariaTest(
// Send key "ARROW_HOME"
await combobox.sendKeys(Key.HOME);

t.true(
await confirmCursorIndex(t, ex.comboboxSelector, 0),
'Cursor should be at index 0 after one ARROW_HOME key'
);
// On macOS, the HOME key deselects the grid popup
// but it doesn't move the cursor.
stalgiag marked this conversation as resolved.
Show resolved Hide resolved
if (!isMacOS()) {
t.true(
await confirmCursorIndex(t, ex.comboboxSelector, 0),
'Cursor should be at index 0 after one ARROW_HOME key'
);
}

t.is(
await combobox.getAttribute('aria-activedescendant'),
Expand Down
4 changes: 4 additions & 0 deletions test/tests/disclosure_navigation_hybrid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,11 @@ ariaTest(

if (links.length > 0) {
await buttons[b].click();

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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


t.is(
await links[0].getAttribute('aria-current'),
Expand Down
7 changes: 5 additions & 2 deletions test/tests/toolbar_toolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const assertAttributeValues = require('../util/assertAttributeValues');
const assertRovingTabindex = require('../util/assertRovingTabindex');
const assertHasFocus = require('../util/assertHasFocus');
const assertAttributeCanBeToggled = require('../util/assertAttributeCanBeToggled');
const translatePlatformKey = require('../util/translatePlatformKey');

const exampleFile = 'content/patterns/toolbar/examples/toolbar.html';

Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I addressed with aa905f6.

await textarea.sendKeys(Key.chord(...modifierKey, 'a'));
let originalText = await textarea.getAttribute('value');

const buttons = await t.context.queryElements(
Expand Down Expand Up @@ -1206,7 +1208,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);
await textarea.sendKeys(Key.chord(...modifierKey, 'a'));
let originalText = await textarea.getAttribute('value');

const buttons = await t.context.queryElements(
Expand Down
8 changes: 8 additions & 0 deletions test/util/isMacOS.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* Returns true if the current platform is macOS
*
* @returns {boolean}
*/
module.exports = function isMacOS() {
return process.platform === 'darwin';
};
26 changes: 26 additions & 0 deletions test/util/translatePlatformKey.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
const { Key } = require('selenium-webdriver');
const isMacOS = require('./isMacOS');

const MAC_KEY_MAPPINGS = {
[Key.CONTROL]: Key.META,
};

/**
* Translates a key or key combination for the current OS
*
* @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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const keyArray = Array.isArray(keys) ? keys : [keys];
if (!isMacOS()) {
return keyArray;
}

return keyArray.reduce((acc, key) => {
const mappedKey = MAC_KEY_MAPPINGS[key] || key;
return acc.concat(mappedKey);
}, []);
}

module.exports = translatePlatformKey;
Loading