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

Jump to bottom

Use supertest instead of repo-tools test app #1416

Merged
merged 2 commits into from
Jul 17, 2019
Merged

Use supertest instead of repo-tools test app #1416

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe2ed3e
Use supertest instead of repo-tools test app
fhinkel Jul 16, 2019
9bbc975
Merge branch 'master' into repoToolsInHelloWorld
fhinkel Jul 16, 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
2 changes: 2 additions & 0 deletions appengine/hello-world/standard/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,5 @@ app.listen(PORT, () => {
console.log('Press Ctrl+C to quit.');
});
// [END gae_node_request_example]

module.exports = app;
6 changes: 4 additions & 2 deletions appengine/hello-world/standard/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,17 @@
"scripts": {
"deploy": "gcloud app deploy",
"start": "node app.js",
"system-test": "repo-tools test app",
"system-test": "mocha --exit test/*.test.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

fun idea which you can take or leave, if you add c8 as a dependency, it turns on Node's V8 coverage collection, so the user gets:

coverage

also understand if you think this is too much information for the user all at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to keep dependencies as light as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But lol, that coverage looks good!

Copy link
Contributor

Choose a reason for hiding this comment

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

The --exit always makes me nervous, but it's not really blocking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests don't finish without --exit

Copy link
Contributor

Choose a reason for hiding this comment

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

The default for --exit was changed relatively recently in mocha:

mochajs/mocha#3034

I think it's reasonable to go back to the old default ... alternatively you'd need to capture the server object returned by app.listen and then call .close() on it when tests are complete... sounds like a hassle 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So by using --exit I'm just using Mocha the way it used to be, right?

"test": "npm run system-test",
"e2e-test": "repo-tools test deploy"
},
"dependencies": {
"express": "^4.16.3"
},
"devDependencies": {
"@google-cloud/nodejs-repo-tools": "^3.3.0"
"@google-cloud/nodejs-repo-tools": "^3.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can delete this dependency now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No :) It's used in the e2e-test. But turns out, we're not calling 2e2-test from CI, instead we have special test files that run a deployment. But I'll prefer deleting it in a separate PR. There's a lot of magic happening in repo-tools based on env variables.

"mocha": "^6.1.4",
"supertest": "^4.0.2"
},
"cloud-repo-tools": {
"test": {
Expand Down
16 changes: 16 additions & 0 deletions appengine/hello-world/standard/test/app.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
const app = require('../app');

const request = require('supertest');

describe('GET /', () => {
it('should get 200', done => {
request(app)
.get('/')
.expect(200, done);
}),
it('should get Hello World', done => {
request(app)
.get('/')
.expect('Hello, world!', done);
});
});