From b042f218a9cfc31c015e6e38af164265c4f58c9f Mon Sep 17 00:00:00 2001 From: m-akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 6 Apr 2023 12:08:14 -0500 Subject: [PATCH] Run Firefox tests on CI and skip existing failing tests (#1156) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## ๐Ÿคจ Rationale Relates to #1075 I fixed 7 of the 17 tests that were failing on Firefox. We want to start running the Firefox tests on the CI (and `npm run test`), but skip the ones known to be failing. ## ๐Ÿ‘ฉโ€๐Ÿ’ป Implementation Malcolm pointed me to a package called `karma-jasmine-spec-tags` which they had used on the WebVI team. This package allows running or skipping tests based on arbitrary tags found in the test names. I've added "#SkipFirefox" to the 10 remaining tests that are still failing on Firefox. I then added `--skip-tags SkipFirefox` to the arguments passed when executing `npm run test-firefox`. I added similar skipping support for Chrome and Webkit. I documented our policy for skipping tests in the CONTRIBUTING. ## ๐Ÿงช Testing Tested that the tagged tests are indeed skipped now, and the rest are being run on the CI. ## โœ… Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed. --------- Co-authored-by: Milan Raj --- ...-788ed1e7-ebcc-469f-ab96-b5e608276b80.json | 7 +++++ package-lock.json | 20 +++++++++++++ packages/nimble-components/CONTRIBUTING.md | 11 ++++++++ packages/nimble-components/karma.conf.js | 16 ++++++++++- packages/nimble-components/package.json | 26 +++++++++++------ .../nimble-components/src/anchor-tab/index.ts | 11 ++++++++ .../src/dialog/tests/dialog.spec.ts | 9 ++++-- .../src/drawer/tests/drawer.spec.ts | 12 +++++--- .../text/tests/table-column-text.spec.ts | 3 +- .../header/tests/table-header.spec.ts | 6 ++-- .../src/tooltip/tests/tooltip.spec.ts | 28 ++++++++++++++----- 11 files changed, 123 insertions(+), 26 deletions(-) create mode 100644 change/@ni-nimble-components-788ed1e7-ebcc-469f-ab96-b5e608276b80.json diff --git a/change/@ni-nimble-components-788ed1e7-ebcc-469f-ab96-b5e608276b80.json b/change/@ni-nimble-components-788ed1e7-ebcc-469f-ab96-b5e608276b80.json new file mode 100644 index 0000000000..f7bd718bb7 --- /dev/null +++ b/change/@ni-nimble-components-788ed1e7-ebcc-469f-ab96-b5e608276b80.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Add ariaSelected property to anchor tab", + "packageName": "@ni/nimble-components", + "email": "7282195+m-akinc@users.noreply.github.com", + "dependentChangeType": "patch" +} diff --git a/package-lock.json b/package-lock.json index 488197ba55..def24a94fb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18137,6 +18137,17 @@ "karma-jasmine": "^5.0.0" } }, + "node_modules/karma-jasmine-spec-tags": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/karma-jasmine-spec-tags/-/karma-jasmine-spec-tags-2.0.0.tgz", + "integrity": "sha512-ckTZvS+w9LyYQtI/LY6nNS6oiuQM7bSRzJgLBwde5Ivr6k5uQ1y58HD77YQH9+rWIj048LXBpXk2VY3ws9hE2A==", + "dev": true, + "peerDependencies": { + "jasmine": ">=4 || >=5", + "karma": ">=6.0.4", + "karma-jasmine": "*" + } + }, "node_modules/karma-source-map-support": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/karma-source-map-support/-/karma-source-map-support-1.4.0.tgz", @@ -28916,6 +28927,7 @@ "karma-firefox-launcher": "^2.1.0", "karma-jasmine": "^5.1.0", "karma-jasmine-html-reporter": "^2.0.0", + "karma-jasmine-spec-tags": "^2.0.0", "karma-source-map-support": "^1.4.0", "karma-sourcemap-loader": "^0.3.7", "karma-spec-reporter": "^0.0.36", @@ -32970,6 +32982,7 @@ "karma-firefox-launcher": "^2.1.0", "karma-jasmine": "^5.1.0", "karma-jasmine-html-reporter": "^2.0.0", + "karma-jasmine-spec-tags": "^2.0.0", "karma-source-map-support": "^1.4.0", "karma-sourcemap-loader": "^0.3.7", "karma-spec-reporter": "^0.0.36", @@ -43339,6 +43352,13 @@ "dev": true, "requires": {} }, + "karma-jasmine-spec-tags": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/karma-jasmine-spec-tags/-/karma-jasmine-spec-tags-2.0.0.tgz", + "integrity": "sha512-ckTZvS+w9LyYQtI/LY6nNS6oiuQM7bSRzJgLBwde5Ivr6k5uQ1y58HD77YQH9+rWIj048LXBpXk2VY3ws9hE2A==", + "dev": true, + "requires": {} + }, "karma-source-map-support": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/karma-source-map-support/-/karma-source-map-support-1.4.0.tgz", diff --git a/packages/nimble-components/CONTRIBUTING.md b/packages/nimble-components/CONTRIBUTING.md index d6c3ade341..3d351ac673 100644 --- a/packages/nimble-components/CONTRIBUTING.md +++ b/packages/nimble-components/CONTRIBUTING.md @@ -387,6 +387,17 @@ Test utilties located in [`/src/utilities/tests`](/packages/nimble-components/sr The jasmine unit tests utilize [`fixture.ts`](/packages/nimble-components/src/utilities/tests/fixture.ts) for component tests. The fixture utility gives tools for managing the component lifecycle. For some usage examples see [`fixture.spec.ts`](/packages/nimble-components/src/utilities/tests/fixture.spec.ts). +### Disabling tests + +If a test is failing on a specific browser but passing on others, it is possible to temporarily mark it to be skipped for that browser by applying the tag `#SkipFirefox`, `#SkipWebkit`, or `#SkipChrome` to the test name: + +```ts +// Firefox skipped, see: https://github.com/ni/nimble/issues/#### +it('sets title when cell text is ellipsized #SkipFirefox', ...); +``` + +Before disabling a test, you **must** have investigated the failure and attempted to find a proper resolution. If you still end up needing to disable it, there must be an issue in this repo tracking the failure, and you must add a comment in the source linking to that issue. + ## Theming Nimble includes three NI-brand aligned themes (i.e. `light`, `dark`, & `color`). diff --git a/packages/nimble-components/karma.conf.js b/packages/nimble-components/karma.conf.js index 1f7d3480bd..f2436708fe 100644 --- a/packages/nimble-components/karma.conf.js +++ b/packages/nimble-components/karma.conf.js @@ -47,10 +47,16 @@ module.exports = config => { basePath, browserDisconnectTimeout: 10000, processKillTimeout: 10000, - frameworks: ['source-map-support', 'jasmine', 'webpack'], + frameworks: [ + 'source-map-support', + 'jasmine', + 'webpack', + 'jasmine-spec-tags' + ], plugins: [ 'karma-jasmine', 'karma-jasmine-html-reporter', + 'karma-jasmine-spec-tags', 'karma-webpack', 'karma-source-map-support', 'karma-sourcemap-loader', @@ -124,6 +130,14 @@ module.exports = config => { ChromeHeadlessOpt: { base: 'ChromeHeadless', flags: [...commonChromeFlags] + }, + FirefoxDebugging: { + base: 'Firefox', + debug: true + }, + WebkitDebugging: { + base: 'Webkit', + debug: true } }, client: { diff --git a/packages/nimble-components/package.json b/packages/nimble-components/package.json index 54b586f21a..13b57b9c16 100644 --- a/packages/nimble-components/package.json +++ b/packages/nimble-components/package.json @@ -26,14 +26,23 @@ "generate-scss:run": "node build/generate-scss/dist/index.js", "tdd": "npm run build-components && npm run test-chrome", "tdd:watch": "npm run build-components:watch & npm run test-chrome:watch", - "test-chrome:debugger": "karma start karma.conf.js --browsers=ChromeDebugging", - "test-chrome:verbose": "karma start karma.conf.verbose.js --browsers=ChromeHeadlessOpt --single-run", - "test-chrome:watch": "karma start karma.conf.js --browsers=ChromeHeadlessOpt --watch-extensions js", - "test-chrome": "karma start karma.conf.js --browsers=ChromeHeadlessOpt --single-run", - "test-firefox": "karma start karma.conf.js --browsers=FirefoxHeadless --single-run", - "test-webkit": "karma start karma.conf.js --browsers=WebkitHeadless --single-run", - "test-webkit:debugger": "karma start karma.conf.js --browsers=Webkit", - "test": "npm run test-chrome:verbose" + "tdd-firefox": "npm run build-components && npm run test-firefox", + "tdd-firefox:watch": "npm run build-components:watch & npm run test-firefox:watch", + "tdd-webkit": "npm run build-components && npm run test-webkit", + "tdd-webkit:watch": "npm run build-components:watch & npm run test-webkit:watch", + "test-chrome:debugger": "karma start karma.conf.js --browsers=ChromeDebugging --skip-tags SkipChrome", + "test-chrome:verbose": "karma start karma.conf.verbose.js --browsers=ChromeHeadlessOpt --single-run --skip-tags SkipChrome", + "test-chrome:watch": "karma start karma.conf.js --browsers=ChromeHeadlessOpt --skip-tags SkipChrome --watch-extensions js", + "test-chrome": "karma start karma.conf.js --browsers=ChromeHeadlessOpt --single-run --skip-tags SkipChrome", + "test-firefox:debugger": "karma start karma.conf.js --browsers=FirefoxDebugging --skip-tags SkipFirefox", + "test-firefox:verbose": "karma start karma.conf.verbose.js --browsers=FirefoxHeadless --single-run --skip-tags SkipFirefox", + "test-firefox:watch": "karma start karma.conf.js --browsers=FirefoxHeadless --skip-tags SkipFirefox --watch-extensions js", + "test-firefox": "karma start karma.conf.js --browsers=FirefoxHeadless --single-run --skip-tags SkipFirefox", + "test-webkit:debugger": "karma start karma.conf.js --browsers=WebkitDebugging --skip-tags SkipWebkit", + "test-webkit:verbose": "karma start karma.conf.verbose.js --browsers=WebkitHeadless --single-run --skip-tags SkipWebkit", + "test-webkit:watch": "karma start karma.conf.js --browsers=WebkitHeadless --skip-tags SkipWebkit --watch-extensions js", + "test-webkit": "karma start karma.conf.js --browsers=WebkitHeadless --single-run --skip-tags SkipWebkit", + "test": "npm run test-chrome:verbose && npm run test-firefox:verbose" }, "repository": { "type": "git", @@ -103,6 +112,7 @@ "karma-firefox-launcher": "^2.1.0", "karma-jasmine": "^5.1.0", "karma-jasmine-html-reporter": "^2.0.0", + "karma-jasmine-spec-tags": "^2.0.0", "karma-source-map-support": "^1.4.0", "karma-sourcemap-loader": "^0.3.7", "karma-spec-reporter": "^0.0.36", diff --git a/packages/nimble-components/src/anchor-tab/index.ts b/packages/nimble-components/src/anchor-tab/index.ts index 9bbf1501bf..82a59482b7 100644 --- a/packages/nimble-components/src/anchor-tab/index.ts +++ b/packages/nimble-components/src/anchor-tab/index.ts @@ -28,6 +28,17 @@ export class AnchorTab extends AnchorBase { */ @attr({ mode: 'boolean' }) public disabled = false; + + /** + * Indicates the current "selected" state of various widgets. + * {@link https://www.w3.org/TR/wai-aria-1.1/#aria-selected} + * + * @public + * @remarks + * HTML Attribute: aria-selected + */ + @attr({ attribute: 'aria-selected' }) + public override ariaSelected = 'false'; } // FoundationAnchor already applies the StartEnd mixin, so we don't need to do it here. diff --git a/packages/nimble-components/src/dialog/tests/dialog.spec.ts b/packages/nimble-components/src/dialog/tests/dialog.spec.ts index df998bc585..1c740f4daf 100644 --- a/packages/nimble-components/src/dialog/tests/dialog.spec.ts +++ b/packages/nimble-components/src/dialog/tests/dialog.spec.ts @@ -242,7 +242,8 @@ describe('Dialog', () => { await disconnect(); }); - it('focuses the first button on the dialog when it opens', async () => { + // Firefox skipped, see: https://github.com/ni/nimble/issues/1075 + it('focuses the first button on the dialog when it opens #SkipFirefox', async () => { const { element, connect, disconnect } = await setup(); await connect(); const okButton = document.getElementById('ok')!; @@ -254,7 +255,8 @@ describe('Dialog', () => { await disconnect(); }); - it('focuses the button with autofocus when the dialog opens', async () => { + // Firefox skipped, see: https://github.com/ni/nimble/issues/1075 + it('focuses the button with autofocus when the dialog opens #SkipFirefox', async () => { const { element, connect, disconnect } = await setup(); await connect(); const cancelButton = document.getElementById('cancel')!; @@ -268,7 +270,8 @@ describe('Dialog', () => { await disconnect(); }); - it('supports opening multiple dialogs on top of each other', async () => { + // Firefox skipped, see: https://github.com/ni/nimble/issues/1075 + it('supports opening multiple dialogs on top of each other #SkipFirefox', async () => { const { element, connect, disconnect } = await setup(); await connect(); const secondDialog = document.createElement('nimble-dialog'); diff --git a/packages/nimble-components/src/drawer/tests/drawer.spec.ts b/packages/nimble-components/src/drawer/tests/drawer.spec.ts index 48c74c401b..f12de2eba3 100644 --- a/packages/nimble-components/src/drawer/tests/drawer.spec.ts +++ b/packages/nimble-components/src/drawer/tests/drawer.spec.ts @@ -130,7 +130,8 @@ describe('Drawer', () => { await expectAsync(promise).toBePending(); }); - it('should resolve promise if drawer completely opens before being closed', async () => { + // Firefox skipped, see: https://github.com/ni/nimble/issues/1075 + it('should resolve promise if drawer completely opens before being closed #SkipFirefox', async () => { const promise = element.show(); await completeAnimationAsync(element); element.close(); @@ -213,13 +214,15 @@ describe('Drawer', () => { expect(afterDrawerCloseActiveElement).toBe(button2); }); - it('focuses the first button on the drawer when it opens', () => { + // Firefox skipped, see: https://github.com/ni/nimble/issues/1075 + it('focuses the first button on the drawer when it opens #SkipFirefox', () => { const okButton = document.getElementById('ok')!; void element.show(); expect(document.activeElement).toBe(okButton); }); - it('focuses the button with autofocus when the drawer opens', () => { + // Firefox skipped, see: https://github.com/ni/nimble/issues/1075 + it('focuses the button with autofocus when the drawer opens #SkipFirefox', () => { const cancelButton = document.getElementById('cancel')!; cancelButton.setAttribute('autofocus', ''); processUpdates(); @@ -227,7 +230,8 @@ describe('Drawer', () => { expect(document.activeElement).toBe(cancelButton); }); - it('supports opening multiple drawers on top of each other', () => { + // Firefox skipped, see: https://github.com/ni/nimble/issues/1075 + it('supports opening multiple drawers on top of each other #SkipFirefox', () => { const secondDrawer = document.createElement('nimble-drawer'); const secondDrawerButton = document.createElement('nimble-button'); secondDrawer.append(secondDrawerButton); diff --git a/packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts b/packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts index 7578348449..eaf25abe9b 100644 --- a/packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts +++ b/packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts @@ -121,7 +121,8 @@ describe('TableColumnText', () => { expect(pageObject.getRenderedCellContent(0, 0)).toBe(''); }); - it('sets title when cell text is ellipsized', async () => { + // Firefox skipped, see: https://github.com/ni/nimble/issues/1075 + it('sets title when cell text is ellipsized #SkipFirefox', async () => { const cellContents = 'a very long value that should get ellipsized due to not fitting within the default cell width'; element.setData([{ field: cellContents }]); await connect(); diff --git a/packages/nimble-components/src/table/components/header/tests/table-header.spec.ts b/packages/nimble-components/src/table/components/header/tests/table-header.spec.ts index a54178b3cb..8ebf72e41d 100644 --- a/packages/nimble-components/src/table/components/header/tests/table-header.spec.ts +++ b/packages/nimble-components/src/table/components/header/tests/table-header.spec.ts @@ -60,7 +60,8 @@ describe('TableHeader', () => { expect(sortIcons.descendingIcon).toBeFalsy(); }); - it('has correct state when sorted ascending', async () => { + // Firefox skipped, see: https://github.com/ni/nimble/issues/1075 + it('has correct state when sorted ascending #SkipFirefox', async () => { element.sortDirection = TableColumnSortDirection.ascending; element.firstSortedColumn = true; await waitForUpdatesAsync(); @@ -71,7 +72,8 @@ describe('TableHeader', () => { expect(sortIcons.descendingIcon).toBeFalsy(); }); - it('has correct state when sorted descending', async () => { + // Firefox skipped, see: https://github.com/ni/nimble/issues/1075 + it('has correct state when sorted descending #SkipFirefox', async () => { element.sortDirection = TableColumnSortDirection.descending; element.firstSortedColumn = true; await waitForUpdatesAsync(); diff --git a/packages/nimble-components/src/tooltip/tests/tooltip.spec.ts b/packages/nimble-components/src/tooltip/tests/tooltip.spec.ts index b15db99866..bb86a89035 100644 --- a/packages/nimble-components/src/tooltip/tests/tooltip.spec.ts +++ b/packages/nimble-components/src/tooltip/tests/tooltip.spec.ts @@ -9,6 +9,14 @@ async function setup(): Promise> { return fixture(html``); } +// For some reason, on Firefox, it takes two calls to waitForUpdatesAsync before +// the icon elements have computed styles. If we call it just once, getComputedStyles() +// returns an empty styles object. +async function waitForIconVisibilityCheck(): Promise { + await waitForUpdatesAsync(); + await waitForUpdatesAsync(); +} + describe('Tooltip', () => { let parent: HTMLElement; let element: Tooltip; @@ -20,7 +28,13 @@ describe('Tooltip', () => { if (!iconElement) { return false; } - return window.getComputedStyle(iconElement).display !== 'none'; + const display = window.getComputedStyle(iconElement).display; + if (display === '') { + throw new Error('Value of display was unexpectedly empty'); + } + return ( + display === 'block' || display === 'inline' || display === 'flex' + ); } beforeEach(async () => { @@ -121,7 +135,7 @@ describe('Tooltip', () => { element.visible = true; await connect(); - await waitForUpdatesAsync(); + await waitForIconVisibilityCheck(); expect(isIconVisible('nimble-icon-exclamation-mark')).toBeFalse(); expect(isIconVisible('nimble-icon-info')).toBeFalse(); @@ -134,7 +148,7 @@ describe('Tooltip', () => { element.iconVisible = true; await connect(); - await waitForUpdatesAsync(); + await waitForIconVisibilityCheck(); expect(isIconVisible('nimble-icon-exclamation-mark')).toBeFalse(); expect(isIconVisible('nimble-icon-info')).toBeFalse(); @@ -147,7 +161,7 @@ describe('Tooltip', () => { element.severity = 'error'; await connect(); - await waitForUpdatesAsync(); + await waitForIconVisibilityCheck(); expect(isIconVisible('nimble-icon-exclamation-mark')).toBeFalse(); expect(isIconVisible('nimble-icon-info')).toBeFalse(); @@ -161,7 +175,7 @@ describe('Tooltip', () => { element.iconVisible = true; await connect(); - await waitForUpdatesAsync(); + await waitForIconVisibilityCheck(); expect(isIconVisible('nimble-icon-exclamation-mark')).toBeTrue(); expect(isIconVisible('nimble-icon-info')).toBeFalse(); @@ -174,7 +188,7 @@ describe('Tooltip', () => { element.severity = 'information'; await connect(); - await waitForUpdatesAsync(); + await waitForIconVisibilityCheck(); expect(isIconVisible('nimble-icon-exclamation-mark')).toBeFalse(); expect(isIconVisible('nimble-icon-info')).toBeFalse(); @@ -188,7 +202,7 @@ describe('Tooltip', () => { element.iconVisible = true; await connect(); - await waitForUpdatesAsync(); + await waitForIconVisibilityCheck(); expect(isIconVisible('nimble-icon-exclamation-mark')).toBeFalse(); expect(isIconVisible('nimble-icon-info')).toBeTrue();