Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#122: Add JavaScript functional test harness #129

Merged
merged 14 commits into from
Aug 20, 2019
Merged

#122: Add JavaScript functional test harness #129

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
999762b
Add JS functional test harness
biancadanforth Aug 5, 2019
43e4a30
Test Fathom's isVisible method on a page
biancadanforth Aug 5, 2019
e01f13f
Add locally hosted test page
biancadanforth Aug 8, 2019
938234e
Update test cases
biancadanforth Aug 10, 2019
8c4d931
Rename functional test and test page
biancadanforth Aug 10, 2019
0b326d9
Move JS functional tests into existing 'JavaScript tests' stage
biancadanforth Aug 10, 2019
210644c
Incorporate initial feedback
biancadanforth Aug 14, 2019
68b791b
Ignore node/no-missing-require eslint error
biancadanforth Aug 14, 2019
8d87435
Fix mocha timeout issue
biancadanforth Aug 19, 2019
c8f2ee2
Exclude 'isVisible' and its called functions from coverage
biancadanforth Aug 20, 2019
ed8e087
Make 'npm test' work both locally and in CI
biancadanforth Aug 20, 2019
9e2ab8e
Rename 'test/functional' to 'test/browser'
biancadanforth Aug 20, 2019
b04d519
Incorporate other review feedback
biancadanforth Aug 20, 2019
a3fa717
Fix #122: Incorporate more feedback.
biancadanforth Aug 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .eslintrc.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
env:
es6: true
node: true
mocha: true

parserOptions:
sourceType: module
ecmaVersion: 7
ecmaVersion: 8

extends:
- eslint:recommended
Expand All @@ -17,6 +18,7 @@ plugins:
root: true

rules:
array-bracket-spacing: [error, never]
eqeqeq: error
generator-star-spacing: [warn, {before: true, after: false}]
guard-for-in: warn # There's nothing wrong with for..in if you know what you're doing. This is here just to keep me from accidentally saying "for..in" when I mean "for..of". Delete this and come up with a better solution if we ever need to use "for..in".
Expand All @@ -36,6 +38,7 @@ rules:
no-useless-escape: error
no-var: warn
no-warning-comments: [warn, {terms: [xxx, fixme, hack], location: start}]
object-curly-spacing: [error, never]
object-shorthand: [error, properties]
prefer-const: off
quotes: [error, single, {avoidEscape: true, allowTemplateLiterals: true}]
Expand Down
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jobs:
firefox: latest
script:
- export PATH=$PATH:$TRAVIS_BUILD_DIR/node_modules/geckodriver/bin
- test/functional/setup_http_server &
- test/functional/start_http_server &
- npm test
- kill %1
after_success:
Expand Down
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ all: $(JS)

lint:
@node_modules/.bin/eslint --ext mjs .
@node_modules/.bin/eslint test/functional
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


test: $(JS)
@node_modules/.bin/mocha --recursive
Expand Down
4 changes: 0 additions & 4 deletions test/.eslintrc.yml
View file
Open in desktop

This file was deleted.

88 changes: 28 additions & 60 deletions test/functional/isVisible.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,86 +2,54 @@
<html lang="en" dir="ltr">
<head>
<meta charset="utf-8">
<link rel="shortcut icon" href="">
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for, anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

gtk! A comment would be great for Future Me.

<link rel="icon" href="data:,">
<title>isVisible functional test</title>
<style type="text/css">
:root {
--size: 100px;
}
#not-visible-1 {
width: 0px;
overflow: hidden;
}
#not-visible-2 {
height: 0px;
overflow: hidden;
}
#not-visible-3 {
display: none;
}
#not-visible-4-ancestor {
display: none;
}
#not-visible-5 {
opacity: 0;
}
#not-visible-6-ancestor {
opacity: 0;
}
#not-visible-7-ancestor {
width: 0;
}
#not-visible-7 {
overflow: hidden;
}
#not-visible-8-ancestor {
height: 0;
}
#not-visible-8 {
overflow: hidden;
}
.off-screen {
width: var(--size);
height: var(--size);
border: 1px solid black;
position: absolute;
}
#not-visible-9 {
left: calc(-2 * var(--size));
}
#not-visible-10 {
top: calc(-2 * var(--size));
}
#not-visible-11 {
right: calc(100vw + 2 * var(--size));
}
#not-visible-12 {
bottom: calc(100vh + 2 * var(--size));
}

</style>
</head>
<body>
<h1>isVisible functional test</h1>
<div id="not-visible-1"></div>
<div id="not-visible-2"></div>
<div id="not-visible-3"></div>
<div id="not-visible-4-ancestor">
<div id="not-visible-1" style="width: 0px; overflow: hidden;"></div>
<div id="not-visible-2" style="height: 0px; overflow: hidden;"></div>
<div id="not-visible-3" style="display: none;"></div>
<!-- ``display: none`` ancestor -->
<div id="ancestor-not-visible-4" style="display: none;">
<div id="not-visible-4"></div>
</div>
<div id="not-visible-5"></div>
<div id="not-visible-6-ancestor">
<div id="not-visible-5" style="opacity: 0;"></div>
<!-- ``opacity: 0`` ancestor -->
<div id="ancestor-not-visible-6" style="opacity: 0;">
<div id="not-visible-6"></div>
</div>
<div id="not-visible-7-ancestor">
<div id="not-visible-7"></div>
<!-- ``overflow: hidden`` with zero width ancestor -->
<div id="ancestor-not-visible-7" style="width: 0;">
<div id="not-visible-7" style="overflow: hidden;"></div>
</div>
<!-- ``overflow: hidden`` with zero height ancestor -->
<div id="ancestor-not-visible-8" style="height: 0;">
<div id="not-visible-8" style="overflow: hidden;"></div>
</div>
<div id="not-visible-8-ancestor">
<div id="not-visible-8"></div>
<!-- off-screen to the left of viewport -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I love how you've commented and inlined these!

<div id="not-visible-9" class="off-screen" style="left: calc(-2 * var(--size));"></div>
<!-- off-screen above viewport -->
<div id="not-visible-10" class="off-screen" style="top: calc(-2 * var(--size));"></div>
<!-- off-screen to the right of viewport -->
<div id="not-visible-11" class="off-screen" style="right: calc(100vw + 2 * var(--size));"></div>
<!-- off-screen below viewport -->
<div id="not-visible-12" class="off-screen" style="bottom: calc(100vh + 2 * var(--size));"></div>
<div id="not-visible-13" style="display: contents"></div>
<!-- ``display: contents`` ancestor -->
<div id="ancestor-visible-1" style="display: contents">
<div id="visible-1"></div>
</div>
<div id="not-visible-9" class="off-screen"></div>
<div id="not-visible-10" class="off-screen"></div>
<div id="not-visible-11" class="off-screen"></div>
<div id="not-visible-12" class="off-screen"></div>
</body>
</html>
71 changes: 36 additions & 35 deletions test/functional/isVisible.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
const { assert } = require('chai');
const {assert} = require('chai');
const firefox = require('selenium-webdriver/firefox');
const webdriver = require('selenium-webdriver');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

const { ancestors, isDomElement, isVisible, toDomElement } = require('../../utilsForFrontend');
const {ancestors, isDomElement, isVisible, toDomElement} = require('../../utilsForFrontend');

const { until, By } = webdriver;
const {Builder, until, By} = webdriver;

const WAIT_MS = 10000;
const TEST_PAGE_URL = 'http://localhost:8000/isVisible.html';
Expand All @@ -12,45 +12,46 @@ describe('isVisible', () => {
const options = new firefox.Options();
options.headless();

const driver = new webdriver.Builder()
const driver = new Builder()
.forBrowser('firefox')
.setFirefoxOptions(options)
.build();

it('Should return false when an element is hidden', async () => {
const hiddenElementIds = [
'not-visible-1',
'not-visible-2',
'not-visible-3',
'not-visible-4',
'not-visible-5',
'not-visible-6',
'not-visible-7',
'not-visible-8',
'not-visible-9',
'not-visible-10',
'not-visible-11',
'not-visible-12',
];
async function checkVisibility(id, expected) {
await driver.wait(until.elementLocated(By.id(id)), WAIT_MS);
const isElementVisible = await driver.executeScript(`
${ancestors}
${isDomElement}
${toDomElement}
return ${isVisible}(document.getElementById('${id}'));
`);
assert.equal(
isElementVisible,
expected,
`isVisible should return false for element with id '${id}'.`
Copy link
Contributor

Choose a reason for hiding this comment

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

false → ${expected}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

);
}

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

Choose a reason for hiding this comment

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

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

return [].slice.call(document.querySelectorAll('[id^="not-visible-"]')).map((element) => element.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

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(...), ...).

`);

await driver.get(TEST_PAGE_URL);
// await driver.wait(async () => {
// const readyState = await driver.executeScript('return document.readyState');
// return readyState === 'complete';
// }, WAIT_MS, `Page did not finish loading after ${WAIT_MS} ms`);

for (const id of hiddenElementIds) {
await driver.wait(until.elementLocated(By.id(id)), WAIT_MS);
const isElementVisible = await driver.executeScript(`
${ancestors}
${isDomElement}
${toDomElement}
return ${isVisible}(document.getElementById('${id}'));
`);
assert.equal(
isElementVisible,
false,
`isVisible should return false for element with id '${id}'.`
);
await checkVisibility(id, false);
}
}).timeout(60000);

it('should return true when an element is visible', async () => {
const visibleElementIds = await driver.executeScript(`
return [].slice.call(document.querySelectorAll('[id^="visible-"]')).map((element) => element.id);
`);
await driver.get(TEST_PAGE_URL);

for (const id of visibleElementIds) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@biancadanforth biancadanforth Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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).

await checkVisibility(id, true);
}
}).timeout(60000);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


Expand Down
4 changes: 1 addition & 3 deletions test/functional/setup_http_server → test/functional/start_http_server
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ const server = http.createServer((request, response) => {
const path = url.parse(request.url).pathname;
fs.readFile(__dirname + path, 'utf8', (error, data) => {
if (error) {
response.writeHead(404);
response.write(`There was an error: ${error.errno}, ${error.code}`);
response.end();
console.error(`There was a ${error.code} error fetching the resource at ${path}.`);
} else {
response.writeHead(200, {'Content-Type': 'text/html'});
response.write(data);
Expand Down