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.

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

Unit tests for service worker remover #32

Merged
merged 13 commits into from
Jul 4, 2019
Merged

Unit tests for service worker remover #32

merged 13 commits into from
Jul 4, 2019

Conversation

prateekbh
Copy link
Member

No description provided.

Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Minor nits, but looks great.

package.json Outdated
@@ -8,8 +8,10 @@
"build:prod": "npm run build -- --public-path='https://cdn.ampproject.org/sw/'",
"transpile": "tsc -p ./src/tsconfig.json",
"pretest": "npm run transpile && npm run build -- --public-path='/test/dist/' && mkdir -p test/dist && cp -R dist/* test/dist",
"test": "node -r esm ./test/index.js",
"posttest": "npm run test:perf",
"test": "yarn test:unit && yarn test:e2e && yarn test:perf",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use && since this won't work on Windows systems. Instead npm-run-all can be used across platforms.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

"test": "yarn test:unit && yarn test:e2e && yarn test:perf",
"test:e2e": "node -r esm ./test/index.js",
"pretest:unit": "tsc -p src",
"test:unit": "node -r esm ./node_modules/.bin/ava ./test/modules/**/unit/*-test.js --verbose",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's verify that es2015+ output requires specifying the esm resolver for node.

Copy link
Member Author

Choose a reason for hiding this comment

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

ava only allows tests files to be written in ES6, any imported utilities/source code has to be in commonjs so using this as a workround

package.json Outdated
@@ -53,6 +55,7 @@
"replace-in-file-webpack-plugin": "1.0.6",
"selenium-assistant": "5.3.0",
"serve-handler": "5.0.7",
"sinon": "^7.3.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's omit the ^.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

client.navigate(client.url),
);
});
async forceRefreshClients(clients: Clients) {
Copy link
Contributor

Choose a reason for hiding this comment

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

On each function definition, please add the type of the return.

async forceRefreshClients(clients: Clients): Promise<void> {

Copy link
Member Author

Choose a reason for hiding this comment

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

done

await clients.claim();
// Cache current document if its AMP.
const windowClients = await clients.matchAll({ type: 'window' });
windowClients.forEach((client: WindowClient) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we specify the return type of the forEach iterator function?

windowClients.forEach((client: WindowClient): void =>

Copy link
Member Author

Choose a reason for hiding this comment

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

done

{"ampRuntimeVersion":"011906051812580","ampCssUrl":"https://cdn.ampproject.org/rtv/011906051812580/v0.css","canaryPercentage":"0.015","diversions":["001906111828200","031906111828200","021906051812580"]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we delete this file or put test/ in the .gitignore.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@prateekbh prateekbh merged commit 58e452b into master Jul 4, 2019
@prateekbh prateekbh deleted the unit-tests branch July 4, 2019 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants