Skip to content

Conversation

fhinkel
Copy link
Contributor

@fhinkel fhinkel commented Jul 16, 2019

Trying to remove repo-tools. Will send a follow up PR that removes repo-tools test deploy.

Ref: #1247

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 16, 2019
@fhinkel
Copy link
Contributor Author

fhinkel commented Jul 16, 2019

@bcoe Can you review this?

@fhinkel fhinkel added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 16, 2019
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 16, 2019
@fhinkel fhinkel force-pushed the repoToolsInHelloWorld branch from 8852e17 to fe2ed3e Compare July 16, 2019 15:38
@fhinkel fhinkel added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 16, 2019
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 16, 2019
Copy link

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

love it 😄 a couple small notes:

  • it's really easy to add test coverage, which might be worth drawing people's attention to (I use this as part of my toolbox).
  • I think we can drop the repo tools dependency.

"deploy": "gcloud app deploy",
"start": "node app.js",
"system-test": "repo-tools test app",
"system-test": "mocha --exit test/*.test.js",
Copy link

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!

},
"devDependencies": {
"@google-cloud/nodejs-repo-tools": "^3.3.0"
"@google-cloud/nodejs-repo-tools": "^3.3.0",
Copy link

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.

@fhinkel fhinkel requested a review from JustinBeckwith July 16, 2019 17:49
"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.

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

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?

@fhinkel fhinkel added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 16, 2019
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 16, 2019
@fhinkel fhinkel merged commit 6e4216f into master Jul 17, 2019
@fhinkel fhinkel deleted the repoToolsInHelloWorld branch July 17, 2019 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants