Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Refactor integration/accessibility tests #22

Merged
merged 7 commits into from
Dec 14, 2018
Merged

Refactor integration/accessibility tests #22

merged 7 commits into from
Dec 14, 2018

Conversation

linuxwolf
Copy link
Contributor

@linuxwolf linuxwolf commented Dec 10, 2018

Connected to #9

This refactor better uses the Selenium-based webdriver and the underlying web-ext API to run integration functional/accessibility tests using the same patterns the unit tests use.

This also adds support for running accessibility testing via axe-webdriverjs (to start, the management homepage).

@ghost ghost assigned linuxwolf Dec 10, 2018
@ghost ghost added the in progress label Dec 10, 2018
@linuxwolf linuxwolf requested a review from a team December 11, 2018 16:59
@jaredhirsch
Copy link
Contributor

@linuxwolf On my mac, I'm not able to get npm run integration to correctly choose Nightly; it defaults to release channel Firefox, then the test fails, because the addon is unsigned. I think the binary needs to be explicitly set in the webdriver code (unless there's a command-line option I haven't discovered).

I found a reference snippet in a semi-recent blog post that seems relevant:

const binary = new firefox.Binary(firefox.Channel.NIGHTLY);
binary.addArguments("--headless");
const driver = new Builder()
  .forBrowser('firefox')
  .setFirefoxOptions(new firefox.Options().setBinary(binary))
  .build();

This doesn't immediately work here, because the firefox() getter inside driver.js causes breakage when trying to access firefox.Binary and firefox.Channel. A little juggling should get it working, though, I think?

@linuxwolf
Copy link
Contributor Author

That's a good catch, and I'll add a way to set which Firefox to look for.

I lean toward making it configurable via environment variables and/or npm config properties, though. For instance, I really want automated testing to run against either unsigned or beta so we're not too bleeding edge.

Copy link
Contributor

@jaredhirsch jaredhirsch left a comment

Choose a reason for hiding this comment

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

LGTM, just a few minor things / questions

test/integration/driver.js Outdated Show resolved Hide resolved
const Cu = Components.utils;
const Services = ChromeUtils.import("resource://gre/modules/Services.jsm");
const { WebExtensionPolicy } = Cu.getGlobalForObject(Services);
return WebExtensionPolicy.getByID("${webext.id}").mozExtensionHostname;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably ask somebody on the addons team (maybe @aswan?) if there's a better incantation to use here, just to be sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point.

This was copied almost verbatim from lockbox-extension's test setup, and was a pattern I saw elsewhere, but it doesn't make it the best.

src/manifest.json.tpl Outdated Show resolved Hide resolved
test/integration/a11y-test.js Outdated Show resolved Hide resolved
@linuxwolf
Copy link
Contributor Author

@mozilla-lockbox/desktop-engineering mind doing another CR? There were some non-trivial changes to address the comments, and another pass seems worthwhile.

@linuxwolf linuxwolf requested a review from a team December 13, 2018 20:51
@linuxwolf linuxwolf removed the request for review from a team December 14, 2018 15:45
@linuxwolf linuxwolf merged commit 2ccae8e into master Dec 14, 2018
@ghost ghost removed the in progress label Dec 14, 2018
@linuxwolf
Copy link
Contributor Author

merged with @6a68 's earlier r+

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants