#122: Add JavaScript functional test harness #129

biancadanforth · 2019-08-10T20:43:33Z

Edit: This PR is branched off of PR #116 , since part of the issue is to write a test for isVisible. It should not be merged until that PR is merged. Only the last 6 commits are new.

Open questions:

I couldn’t figure out how to avoid using timeout in mocha in the test, as it exceeds the default 2s allowed. Using it works, but I'm open to alternatives.
Everything's working except the istanbul coverage.
- If I replace node_modules/.bin/_mocha with node_modules/.bin/mocha in the Makefile's coverage command, my test passes but coverage fails (current state).[1] If I leave node_modules/.bin/_mocha as-is, my test fails, but coverage works.[2]
- I have no idea why these would behave differently or create this trade-off. I think the best course of action may be to update istanbul, since we are using a deprecated version (strangely, that repo says it's at version 0.4.5, not 1.1.0..., but my main point is that there's a v2). @erikrose , @danielhertenstein , would either of you have any ideas/suggestions?

[1]: TravisCI log with node_modules/.bin/mocha in make coverage:

No coverage information was collected, exit without writing coverage information

I found this related issue, which is probably why _mocha was there in the first place.

[2]: TravisCI log (error) with node_modules/.bin/_mocha in make coverage:

  1 failing

  1) isVisible unprivileged

       Should return false when an element is hidden:

     JavascriptError: ReferenceError: cov_1nb0mc0zi8 is not defined
      at Object.throwDecodedError (node_modules/selenium-webdriver/lib/error.js:550:15)
      at parseHttpResponse (node_modules/selenium-webdriver/lib/http.js:560:13)
      at Executor.execute (node_modules/selenium-webdriver/lib/http.js:486:26)
      at processTicksAndRejections (internal/process/task_queues.js:85:5)
      at async thenableWebDriverProxy.execute (node_modules/selenium-webdriver/lib/webdriver.js:699:17)
      at async Context.<anonymous> (test/functional/isVisible.js:41:38)

I tried the /* istanbul ignore next */ solution suggested here, but my test continued to fail no matter where I put it.

erikrose

Great to have some in-browser tests—and without too much pain in the setup. Thanks, Bianca! :-D

I'm not sure what to tell you about the istanbul weirdness. Try upgrading it and see if that helps. If that fails, I'll think harder. Hopefully we won't have to make a tradeoff between coverage and better testing, but if we do, I'll probably choose better testing. (What, after all, is the point of coverage if not to lead to better testing?)

erikrose · 2019-08-12T16:31:03Z

test/functional/setup_http_server

+        if (error) {
+            response.writeHead(404);
+            response.write(`There was an error: ${error.errno}, ${error.code}`);
+            response.end();


Looks like you could factor this out.

erikrose · 2019-08-12T16:37:03Z

test/functional/isVisible.html

+  </head>
+  <body>
+    <h1>isVisible functional test</h1>
+    <div id="not-visible-1"></div>


I think we could reorganize this to make it more comprehensible.

When I first first encounter each tag, I have to look upward to find out what the corresponding style info actually is. Does choosing not to inline the styles actually give us anything? I don't think we are reusing any of the numbered ones. I do like that you factored out .off-screen.

Also, it would be great to have some comments, descriptive names, or something for some of the less obvious ones, like the various calcs.

erikrose · 2019-08-12T16:37:47Z

test/functional/isVisible.js

+const { assert } = require('chai');
+const firefox = require('selenium-webdriver/firefox');
+const webdriver = require('selenium-webdriver');
+const { ancestors, isDomElement, isVisible, toDomElement } = require('../../utilsForFrontend');


The rest of the codebase has no spaces inside the braces. That would be a good linter rule.

Edit: I also updated the ecmaVersion in eslintrc.yml to 8, due to this error otherwise:

/Users/bdanforth/Projects/fathom/test/functional/isVisible.js 20:11 error Parsing error: Unexpected token function

I added the eslint rules for this; also I noticed my test files weren't being linted, so I amended make lint. Unfortunately, I wasn't able to get the .mjs and my test/functional .js files linted in a single eslint command, so I added a second command.

erikrose · 2019-08-12T16:39:45Z

test/functional/isVisible.js

+        .setFirefoxOptions(options)
+        .build();
+
+    it('Should return false when an element is hidden', async () => {


So far, the rest of the codebase starts it( operands with a lowercase letter. Those all show up in the test output, so they're pretty conspicuous.

erikrose · 2019-08-12T16:40:37Z

test/functional/isVisible.js

+
+    it('Should return false when an element is hidden', async () => {
+        const hiddenElementIds = [
+            'not-visible-1',


Might as well just use a counter rather than error-prone-ly typing all these out, no?

erikrose · 2019-08-12T16:42:56Z

test/functional/isVisible.js

+const webdriver = require('selenium-webdriver');
+const { ancestors, isDomElement, isVisible, toDomElement } = require('../../utilsForFrontend');
+
+const { until, By } = webdriver;


Any reason you chose not to do just const {Builder, By, until} = require('selenium-webdriver'); and save a line? Did you especially like the explicitness of webdriver.Builder below?

erikrose · 2019-08-12T16:44:07Z

test/functional/isVisible.js

+        for (const id of hiddenElementIds) {
+            await driver.wait(until.elementLocated(By.id(id)), WAIT_MS);
+            const isElementVisible = await driver.executeScript(`
+                ${ancestors}


Now this is a cool trick.

erikrose · 2019-08-12T16:44:49Z

test/functional/isVisible.js

+        for (const id of hiddenElementIds) {
+            await driver.wait(until.elementLocated(By.id(id)), WAIT_MS);
+            const isElementVisible = await driver.executeScript(`
+                ${ancestors}


JavaScript™: Code-wheelies as a best practice. ;-)

erikrose · 2019-08-12T16:46:25Z

test/functional/isVisible.js

+            assert.equal(
+                isElementVisible,
+                false,
+                `isVisible should return false for element with id '${id}'.`


We should also test at least one case in which isVisible returns true; right now, function() {return false;} would pass all the tests. :-)

Excellent point.

erikrose · 2019-08-12T16:47:41Z

test/functional/setup_http_server

@@ -0,0 +1,24 @@
+#!/usr/bin/env node


A grammar nit: "setup" is a noun, "set up" is a verb. Thus, this should be named set_up_http_server. :-)

Commenting out other stages in travis.yml for now. This just has all the wiring to get a functional test running in TravisCI using Selenium Webdriver, the latest release version of Firefox and the Mocha test framework.

Travis now runs a local HTTP server[1] in a background process, and the functional test opens a locally hosted test page. I also moved all functional test files to a new directory: test/functional Finally, since test scripts are never going to be imported as ES6 modules, I made the test a .js file rather than a .jsm. Note: if I were to leave it as a .jsm, I would have wanted to change the 'require' statements to 'import' statements to keep consistent with the other .mjs files. These .mjs files get transpiled via Babel into CommonJS .js files (i.e. using 'require' statements) as a build step. [1]: I did quite a lot of deliberation about whether an HTTP server would be sufficient (compared to an HTTPS server) and determined it was. Ultimately, I adhered to one of the principles of extreme programming (thanks Daniel): don't add functionality until it's necessary. Here is my reasoning (I spoke with a Mozilla security engineer who works on certificates): * TravisCI's default environment runs each build on a [single machine](https://docs.travis-ci.com/user/reference/trusty/#using-trusty) * This means we are running the server on the loopback interface, and there shouldn't be much risk -- particularly since we don't have any secrets. * The only downside is there are some [DOM APIs that only work in secure contexts](https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts/features_restricted_to_secure_contexts) -- none of which we are using currently. But Firefox is working on enabling these APIs in this case where everything is on the same machine.

Also replace chai's expect with assert for consistency with existing JS tests. For some reason, in the Makefile, make coverage was using node_modules/.bin/_mocha. The isVisible functional test fails unless I replace '_mocha' with 'mocha'.

Now that we are actually linting the isVisible.js functional test, eslint reports an error: ``` /home/travis/build/mozilla/fathom/test/functional/isVisible.js 4:68 error "../../utilsForFrontend" is not found node/no-missing-require ``` I believe this is because 'require' is relative to the directory in which the script is found, while eslint is checking relative to the current working directory. This could be resolved by: * Ignoring the error (easiest) * Updating 'eslint-plugin-node' to >= v5.1.0 in which we can apply the 'resolvePaths' setting. * This requires also updating 'eslint', as otherwise 'make lint' fails * Updating 'eslint' updates default rules, for which there are then dozens of new violations throughout the source code to address * I vote that considering updating eslint be a follow-up issue, rather than part of fixing #122 In light of these options, I chose the best, easiest one, since it doesn't blow up everything else.

biancadanforth · 2019-08-14T04:36:06Z

Well, that's interesting that this exact same branch would fail here and not in my fathom fork. Anyway, assuming I fix that timing issue some time soon, I also need to do the following two things before removing the "Draft" flag:

Move the JS functional test work back to its own stage, since coverage doesn't work when executing code via driver.executeScript
Update the npm test script, so that the functional tests can also be run locally (geckodriver needs to be added to the $PATH and the HTTP server needs to start up/shut down)

erikrose

Looking really good. Couple last ideas.

erikrose · 2019-08-14T13:24:11Z

Makefile

@@ -8,6 +8,7 @@ all: $(JS)

 lint:
 	@node_modules/.bin/eslint --ext mjs .
+	@node_modules/.bin/eslint test/functional


What do you think about calling these "browser" instead of "functional"? I could see us doing unit-style testing of routines that happen to need to run in the browser.

Due to the directory rename, the isVisible test now runs first in the mocha --recursive command. Not a problem, just an observation.

erikrose · 2019-08-14T14:07:21Z

test/functional/isVisible.html

@@ -2,86 +2,54 @@
 <html lang="en" dir="ltr">
  <head>
    <meta charset="utf-8">
-    <link rel="shortcut icon" href="">


What's this for, anyway?

Firefox seems to, by default, make a GET request to get the page's favicon for display in the tab. If I don't have a favicon, my HTTP server throws an error.

This is an empty favicon to prevent that error. I can make a comment saying as much.

gtk! A comment would be great for Future Me.

erikrose · 2019-08-14T14:18:33Z

test/functional/isVisible.html

    </div>
-    <div id="not-visible-8-ancestor">
-      <div id="not-visible-8"></div>
+    <!-- off-screen to the left of viewport -->


I love how you've commented and inlined these!

erikrose · 2019-08-14T14:19:55Z

test/functional/isVisible.js

@@ -1,9 +1,9 @@
-const { assert } = require('chai');
+const {assert} = require('chai');
 const firefox = require('selenium-webdriver/firefox');
 const webdriver = require('selenium-webdriver');


You never actually use webdriver except to grab more imports out of it. Can you do it all in 1 line?

erikrose · 2019-08-14T14:21:16Z

test/functional/isVisible.js

+        assert.equal(
+            isElementVisible,
+            expected,
+            `isVisible should return false for element with id '${id}'.`


false → ${expected}

I am going to guess what you are saying here, since you have chosen not to use words... heh.

This assertion is not executing inside the page like the driver.executeScript call before it. Therefore, I have access to the scope of the checkVisibility function, where expected is defined.

Also, the first and second arguments into assert.equal() are just variables, not template literals.

If I'm missing something, please let me know.

Sorry, I failed to make myself clear! What I meant was that the error message assert.equal takes is assuming that expected is always false. Shouldn't we say isVisible should return ${expected} instead, in case it ever needs to expect true?

Oh; yes the error message if the check fails. Thanks for the catch.

erikrose · 2019-08-14T14:22:35Z

test/functional/isVisible.js

+    }
+
+    it('should return false when an element is hidden', async () => {
+        const hiddenElementIds = await driver.executeScript(`


Now that's a nice piece of DRY. :-) The knowledge of how many hidden elements there is lives in only one place: the markup.

erikrose · 2019-08-14T14:29:40Z

test/functional/isVisible.js

+
+    it('should return false when an element is hidden', async () => {
+        const hiddenElementIds = await driver.executeScript(`
+            return [].slice.call(document.querySelectorAll('[id^="not-visible-"]')).map((element) => element.id);


Is the slice just to copy the results of querySelectorAll into an Array? If so, Array.from(querySelectorAll(...)) might show your intention more clearly.

Shorter still, you could do Array.prototype.map.call(querySelectorAll(...), ...).

erikrose · 2019-08-14T14:31:15Z

test/functional/isVisible.js

+        await driver.get(TEST_PAGE_URL);
+
+        for (const id of visibleElementIds) {
+            await checkVisibility(id, true);
        }
    }).timeout(60000);


Btw, I like that you gave isVisible.html and isVisible.js the same root names. That'll make it easier for Future People to untangle that they go together.

erikrose · 2019-08-14T14:32:26Z

test/functional/isVisible.js

+        `);
+        await driver.get(TEST_PAGE_URL);
+
+        for (const id of visibleElementIds) {


Maybe we should even factor all this stuff up (the Array.prototype.... call, the for loop, and everything). It's nontrivial.

Hmm... not sure I'm following. Can you clarify?

Oh I think I see what you are saying -- make a new function that each it block calls that gets the ids and checks each element's visibility.

I couldn't factor out this.timeout(), because it's only defined in the callback function for each it block (and I need to increase the time for each block, not the overall test).

Added a timeout to the 'after all' hook, as it is possible this can take longer than two seconds. Also made the callbacks into 'it' regular functions instead of arrow functions for consistency with the 'after all' hook. * With arrow functions, the 'this' binding for 'this.timeout' does not point to the desired target, and the 'after' hook threw an error if I tried to use the original approach I had with the 'it' blocks.

Add '/* istanbul ignore next */' to stop trying to cover the following methods, since they are executed in the 'isVisible.html' test page, and that scope does not have coverage variables defined. * ancestors * isDomElement * isVisible * toDomElement This allows coverage to continue to work (ignoring these functions) and keeps the JS functional tests in the same stage as the rest of the JavaScript tests in TravisCI. It should be noted that none of these functions previously had test coverage, which is part of why this approach is feasible.

Move the set up and tear down commands inside of 'npm test' instead of just in '.travis.yml'.

biancadanforth · 2019-08-20T04:30:06Z

@erikrose : This is ready for another look!

I ended up being able to ignore isVisible and its called functions with istanbul, so I didn't end up moving these functional tests to their own stage. Coverage and the tests are now both working together, which seems preferable. Let me know what you think.

erikrose

That's all I've got. If you agree, feel free to make the changes and merge without another round of review. It'll be great to have the ability to test in-browser!

erikrose · 2019-08-20T15:47:19Z

test/functional/isVisible.js

+        assert.equal(
+            isElementVisible,
+            expected,
+            `isVisible should return false for element with id '${id}'.`


Sorry, I failed to make myself clear! What I meant was that the error message assert.equal takes is assuming that expected is always false. Shouldn't we say isVisible should return ${expected} instead, in case it ever needs to expect true?

erikrose · 2019-08-20T16:07:36Z

package.json

    "pretest": "npm run lint",
-    "test": "make coverage"
+    "test": "npm run add-geckodriver-to-path && npm run init-server && make coverage && npm run uninit-server",


You can actually keep the shell environment cleaner by avoiding the export: if you say this instead, the shell variable change will be in scope only for this one line:

PATH=$PATH:$PWD/node_modules/geckodriver/bin npm run add-geckodriver-to-path && npm run init-server && make coverage && npm run uninit-server

The only trouble is that none of this shell cleverness will work under Windows, where @danielhertenstein lives. I can't really think of a way to make it work, either, without a lot of extra code. Let's leave it like this for now and see if it causes him trouble. After all, make test should still run all but the browser tests, and he does have the option of falling back to a WSL shell if he wants to run those too.

You can actually keep the shell environment cleaner by avoiding the export: if you say this instead, the shell variable change will be in scope only for this one line:

PATH=$PATH:$PWD/node_modules/geckodriver/bin npm run add-geckodriver-to-path && npm run init-server && make coverage && npm run uninit-server

Did you mean to remove npm run add-geckodriver-to-path? This is what I have now that is working:

diff --git a/package.json b/package.json index bb87939..fa2a73c 100644 --- a/package.json +++ b/package.json @@ -45,12 +45,11 @@ "url": "https://github.com/mozilla/fathom.git" }, "scripts": { - "add-geckodriver-to-path": "export PATH=$PATH:$PWD/node_modules/geckodriver/bin", "coveralls": "cat ./coverage/lcov.info | coveralls", "lint": "make lint", "init-server": "./test/browser/start_http_server & echo $! > pid", "pretest": "npm run lint", - "test": "npm run add-geckodriver-to-path && npm run init-server && make coverage && npm run uninit-server", + "test": "PATH=$PATH:$PWD/node_modules/geckodriver/bin && npm run init-server && make coverage && npm run uninit-server", "uninit-server": "kill $(cat pid) && rm pid" }, "main": "index"

The only trouble is that none of this shell cleverness will work under Windows, where @danielhertenstein lives. I can't really think of a way to make it work, either, without a lot of extra code. Let's leave it like this for now and see if it causes him trouble. After all, make test should still run all but the browser tests, and he does have the option of falling back to a WSL shell if he wants to run those too.

Ah that's a good point. Actually, I had previously added the --recursive option to the make test command, which means it would try to run the JS functional tests at test/browser as well. It breaks currently, since I'm not doing the requisite set-up and clean-up. I'll remove that option for now, so make test doesn't run the JS functional tests at all. I'll add a comment to note this distinction in the Makefile.

erikrose · 2019-08-20T18:35:13Z

Lgtm!

erikrose · 2019-08-20T18:35:35Z

Please mention "Fix #122" in the merge commit when you merge this.

erikrose suggested changes Aug 12, 2019

View reviewed changes

biancadanforth added 7 commits August 13, 2019 16:04

Add JS functional test harness

999762b

Commenting out other stages in travis.yml for now. This just has all the wiring to get a functional test running in TravisCI using Selenium Webdriver, the latest release version of Firefox and the Mocha test framework.

Test Fathom's isVisible method on a page

43e4a30

Update test cases

938234e

Rename functional test and test page

8c4d931

Incorporate initial feedback

210644c

biancadanforth force-pushed the 122-js-test-harness branch from 7f7de94 to 210644c Compare August 14, 2019 02:29

erikrose suggested changes Aug 14, 2019

View reviewed changes

biancadanforth added 5 commits August 19, 2019 16:58

Make 'npm test' work both locally and in CI

ed8e087

Move the set up and tear down commands inside of 'npm test' instead of just in '.travis.yml'.

Rename 'test/functional' to 'test/browser'

9e2ab8e

Incorporate other review feedback

b04d519

biancadanforth marked this pull request as ready for review August 20, 2019 04:27

biancadanforth requested a review from erikrose August 20, 2019 04:28

erikrose suggested changes Aug 20, 2019

View reviewed changes

Fix #122: Incorporate more feedback.

a3fa717

biancadanforth merged commit 099c8f9 into master Aug 20, 2019

biancadanforth deleted the 122-js-test-harness branch August 20, 2019 18:40

biancadanforth mentioned this pull request Aug 21, 2019

Move istanbul ignore comments #137

Merged

#122: Add JavaScript functional test harness #129

#122: Add JavaScript functional test harness #129

Conversation

biancadanforth commented Aug 10, 2019 • edited Loading

Open questions:

erikrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth Aug 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth commented Aug 14, 2019 • edited Loading

erikrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

biancadanforth commented Aug 20, 2019

erikrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikrose commented Aug 20, 2019

erikrose commented Aug 20, 2019

biancadanforth commented Aug 10, 2019 •

edited

Loading

biancadanforth Aug 14, 2019 •

edited

Loading

biancadanforth commented Aug 14, 2019 •

edited

Loading

biancadanforth Aug 20, 2019 •

edited

Loading