Skip to content

Commit

Permalink
Run Firefox tests on CI and skip existing failing tests (#1156)
Browse files Browse the repository at this point in the history
## 🤨 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 <[email protected]>
  • Loading branch information
m-akinc and rajsite authored Apr 6, 2023
1 parent 4794d9f commit b042f21
Showing 11 changed files with 123 additions and 26 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Add ariaSelected property to anchor tab",
"packageName": "@ni/nimble-components",
"email": "[email protected]",
"dependentChangeType": "patch"
}
20 changes: 20 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions packages/nimble-components/CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -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`).
16 changes: 15 additions & 1 deletion packages/nimble-components/karma.conf.js
Original file line number Diff line number Diff line change
@@ -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: {
26 changes: 18 additions & 8 deletions packages/nimble-components/package.json
Original file line number Diff line number Diff line change
@@ -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",
11 changes: 11 additions & 0 deletions packages/nimble-components/src/anchor-tab/index.ts
Original file line number Diff line number Diff line change
@@ -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.
9 changes: 6 additions & 3 deletions packages/nimble-components/src/dialog/tests/dialog.spec.ts
Original file line number Diff line number Diff line change
@@ -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');
12 changes: 8 additions & 4 deletions packages/nimble-components/src/drawer/tests/drawer.spec.ts
Original file line number Diff line number Diff line change
@@ -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,21 +214,24 @@ 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();
void element.show();
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);
Original file line number Diff line number Diff line change
@@ -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();
Original file line number Diff line number Diff line change
@@ -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();
28 changes: 21 additions & 7 deletions packages/nimble-components/src/tooltip/tests/tooltip.spec.ts
Original file line number Diff line number Diff line change
@@ -9,6 +9,14 @@ async function setup(): Promise<Fixture<Tooltip>> {
return fixture<Tooltip>(html`<nimble-tooltip></nimble-tooltip>`);
}

// 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<void> {
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();

0 comments on commit b042f21

Please sign in to comment.