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

Initial commit adding integration tests for the webextension. #1332

Merged
merged 12 commits into from Sep 10, 2018

Conversation

jrbenny35
Copy link
Contributor

Hello all! 👋🏽

This PR adds some integration tests to test the webextension on each commit or PR. This would take some load off of SoftVision allowing them to test more important features.

Configuration

These tests use Mocha as the test runner and selenium-webdriver to control the Firefox browser. They are written in javascript with a Page-Object Model. The tests will build and install a webextension based on the current changes within the repo/branch when the tests are run.

Tests

  1. should have a list of notes: Checks to make sure the default note is displayed on the notes list page.
  2. should have a default note named correctly: Checks that the default note is named correctly.
  3. should add a note: Checks that you can add a note and it is displayed on the notes list page.
    I have included winston as the logger but have disabled it by default. This can be enabled using the environment variable process.env.UI_TEST_LOGGING = info.

I have also updated the travis.yml file to add the tests to run on travis.

I don't write much javascript but I tried to get caught up and write these with current practices. Any feedback would be greatly appreciated.

@ghost
Copy link

ghost commented Jul 31, 2018

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

5 similar comments
@ghost
Copy link

ghost commented Jul 31, 2018

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@ghost
Copy link

ghost commented Jul 31, 2018

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@ghost
Copy link

ghost commented Jul 31, 2018

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@ghost
Copy link

ghost commented Jul 31, 2018

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@ghost
Copy link

ghost commented Jul 31, 2018

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

const { createLogger, transports } = require('winston');
const { format } = require('logform');
const logger = createLogger({
level: process.env.UI_TEST_LOGGING ? process.env.UI_TEST_LOGGING : 'silent',
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: you may be able to just use level: process.env.UI_TEST_LOGGING || 'silent'

* @throws ElementNotFound
*/
async waitForPageToLoad(locator) {
element = await this.findElement(locator);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure where element is defined, if its leaking into global scope, or if this should say

const element = await this.findElement(locator);

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 was using it just for that method, so I guess a var should work.

await this.driver.wait(this
.until
.elementIsVisable(element), this.timeout);
return this
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: a few of these lines are missing the trailing ;. It isn't failing ESLint, but maybe we should tweak the ESLint config to enforce consistent semicolon 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.

darn semicolons

if (this.root) {
let root = this.driver.findElement(this.by.css(this.root))
root.findElement(this.by.css(locator))
return element
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this element defined? Or was the the previous mentioned leaky variable?

if (this.root) {
let root = this.driver.findElement(this.by.css(this.root))
root.findElements(this.by.css(locator))
return elements
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, where is elements coming from?

var chai = require('chai');
var firefox = require('selenium-webdriver/firefox');
var assert = chai.assert;
var expect = chai.expect;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: these should probably be consts instead of vars.

@@ -0,0 +1,86 @@
const {Builder, By, until} = require('selenium-webdriver');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if these should be require()s (L1, L3-L4) versus an import like on L8 below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain the differences a bit?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not any real big differences. require() is kind of the olde Node.js standard, whereas import is the new module syntax (which you can probably use because you're using babel to transpile the code).
Not an issue either way, just a minor consistency nit which can safely be ignored.

let options;
let timeout = 10000;

this.timeout(20000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be this.timeout(timeout)? Or why is this timeout value different than the variable version on L14?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

timeout is for my page/object model, this.timeout is for Mocha.

timeout,
"The note title was not correct!"
)
expect(noteTitle).to.equal("Welcome to Firefox Notes!")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we've duplicated the title in 2 places, we should consider moving the text into a variable.

Also, not sure if we need both the await driver.wait(...) and the expect(noteTitle).to.equal(...). Looks like they almost verify the same thing.

return browser.getTitle() === 'Welcome to Lockbox';
}, 5000, 'expected text to be different after 5s');
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feels like a placeholder test. Not sure if we can delete this file.

@ghost
Copy link

ghost commented Aug 14, 2018

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@ghost
Copy link

ghost commented Aug 15, 2018

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

addon
);
await driver.setContext('content');
//load index html twice, as once seems to not load the app correctly
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sounds like a bug somewhere

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. I just wanted to see if it was the same on CI. I'll file something

@@ -106,6 +111,8 @@
"start-deved": "npm run webpack && web-ext run -s build --firefox=firefoxdeveloperedition & npm run webpack:watch",
"test": "npm-run-all test:*",
"test:karma": "webpack --config webpack.test-unit.js && NODE_ENV=test karma start",
"test:ui": "test/integration/setup-webext.sh && npm run ui-tests-build && mocha ./test/dist/integration --retries 1 --no-deprecation --reporter=list --compilers js:babel-core/register --require babel-polyfill",
Copy link
Contributor

Choose a reason for hiding this comment

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

@jrbenny35 I get the following

 Error: The geckodriver executable could not be found on the current PATH. Please download the latest version from https://github.com/mozilla/geckodriver/releases/ and ensure it can be found on your PATH.

Could we get something like https://github.com/vladikoff/node-geckodriver into the process so it "just works" when I run test:ui ?

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 is assuming the user is running on Nightly as it seems that plugin keeps up with the most recent releases without a way to install an older version. I'd rather point the user to the geckodriver install and they can install whichever version they need according to their Firefox version.

We could do a docker image, or something like that maybe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm , I think geckodriver 0.21.0 is backwards compatible, our other Selenium infrastructure didn't have issues running in Stable or Nightly. Especially last couple of versions, don't think they have too many changes

Copy link
Contributor

Choose a reason for hiding this comment

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

@jrbenny35 any ideas about:

FIREFOX_BINARY=/Applications/FirefoxNightly.app/Contents/MacOS/firefox npm run test:ui

> notes@4.3.0-dev test:ui /Users/vladikoff/mozilla/notes
> test/integration/setup-webext.sh && npm run ui-tests-build && mocha ./test/dist/integration --retries 1 --no-deprecation --reporter=list --compilers js:babel-core/register --require babel-polyfill

Webextension built and moved to root dir.

> notes@4.3.0-dev ui-tests-build /Users/vladikoff/mozilla/notes
> babel ./test/integration --quiet --no-babelrc --presets=env -d ./test/dist/integration


  1) The Firefox Notes web extension "before each" hook for "should have a default note named correctly"

  0 passing (3s)
  1 failing

  1) The Firefox Notes web extension "before each" hook for "should have a default note named correctly":
     WebDriverError: Could not install add-on: /var/folders/cv/w3cp7lkj6bq70z90s51jllsc0000gn/T/addon-ee6daf6a-c8ae-4451-9bdc-2893432b63f5.xpi: ERROR_SIGNEDSTATE_REQUIRED: The addon must be signed and isn't.
      at Object.throwDecodedError (node_modules/selenium-webdriver/lib/error.js:550:15)
      at parseHttpResponse (node_modules/selenium-webdriver/lib/http.js:542:13)
      at Executor.execute (node_modules/selenium-webdriver/lib/http.js:468:26)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)


Copy link
Contributor

Choose a reason for hiding this comment

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

It's not starting Nightly for me locally

Copy link
Contributor Author

@jrbenny35 jrbenny35 Aug 22, 2018

Choose a reason for hiding this comment

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

Hm , I think geckodriver 0.21.0 is backwards compatible, our other Selenium infrastructure didn't have issues running in Stable or Nightly. Especially last couple of versions, don't think they have too many changes

They kind of work but if someone is using 57, it wouldn't work.

For that error, what is your selenium version?

Copy link
Contributor

Choose a reason for hiding this comment

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

I installed the stuff package.json. i.e. "selenium-webdriver": "^4.0.0-alpha.1",

do we need some kind of a selenium launcher or do i need to manually run it?

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 interesting as I have the preference set to ignore signatures.

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 doesn't look right though: WebDriverError: Could not install add-on: /var/folders/cv/w3cp7lkj6bq70z90s51jllsc0000gn/T/addon-ee6daf6a-c8ae-4451-9bdc-2893432b63f5.xpi, the xpi should be named firefox_notes.xpi.

@jrbenny35
Copy link
Contributor Author

@vladikoff Have you been able to get the tests runnig again?

@vladikoff
Copy link
Contributor

@vladikoff Have you been able to get the tests runnig again?

I think I got stuck locally because the test script doesn't pick up FIREFOX_BINARY= ... when I directly run with that. If I set it to Nightly, it still starts Stable

@jrbenny35
Copy link
Contributor Author

I think I got stuck locally because the test script doesn't pick up FIREFOX_BINARY= ... when I directly run with that. If I set it to Nightly, it still starts Stable

Can you try renaming your FirefoxNightly to just Firefox?

@jrbenny35
Copy link
Contributor Author

I think I got stuck locally because the test script doesn't pick up FIREFOX_BINARY= ... when I directly run with that. If I set it to Nightly, it still starts Stable

@vladikoff any updates?

@vladikoff
Copy link
Contributor

@jrbenny35 I cannot get the tests to run locally, but if they are passing on CI, i think that's fine for now

@vladikoff vladikoff merged commit 103d36f into mozilla:master Sep 10, 2018
@jrbenny35
Copy link
Contributor Author

@jrbenny35 I cannot get the tests to run locally, but if they are passing on CI, i think that's fine for now

Would a vidyo meeting sometime be useful? I'd like you to see them working

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.

None yet

3 participants