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

[DO NOT MERGE] Update/add tests + fix dependencies #452

Closed
wants to merge 22 commits into from
Closed

Conversation

ace-n
Copy link
Contributor

@ace-n ace-n commented Aug 10, 2017

Do not merge until this PR is included in a tagged version of nodejs-repo-tools. (The tests will fail otherwise.)

Also - this is definitely not the last PR on this topic. (There will probably be 2-4 more as I context-switch between things.)

@ace-n ace-n requested a review from jmdobry August 10, 2017 22:01
@codecov
Copy link

codecov bot commented Aug 10, 2017

Codecov Report

Merging #452 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #452      +/-   ##
==========================================
- Coverage   83.92%   83.84%   -0.08%     
==========================================
  Files           4        4              
  Lines         423      421       -2     
==========================================
- Hits          355      353       -2     
  Misses         68       68
Impacted Files Coverage Δ
vision/detect.js 90.61% <0%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60bbf26...d3941bc. Read the comment docs.

@ace-n
Copy link
Contributor Author

ace-n commented Aug 11, 2017

Note/Reminder: appengine/pubsub can't be completely tested locally (because it relies on a POST from Cloud Pub/Sub to receive new messages), and will likely require its own nightly build.

"@google-cloud/nodejs-repo-tools": "1.4.15",
"ava": "0.19.1"
},
"cloud-repo-tools": {
Copy link
Member

Choose a reason for hiding this comment

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

tldr; from our convo, revert changes to "cloud-repo-tools" and the 2 new .js test files, and instead update circle.yml

const process = require('process'); // Required for mocking environment variables

// Make sure starting directory is consistent, so that views are found correctly
process.chdir(__dirname);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this wonkiness, explicitly tell express where to look for views.

@@ -2,16 +2,17 @@
"name": "appengine-storage",
"description": "Node.js Google Cloud Storage sample for Google App Engine",
"scripts": {
"start": "node app.js"
"start": "node app.js",
"test": "ava system-test/*.test.js -T 30s"
Copy link
Member

Choose a reason for hiding this comment

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

Change this to:

"test": "GOOGLE_STORAGE_BUCKET=$(uuidgen) samples test app && ava system-test/*.test.js -T 30s"

AND

add a "cloud-repo-tools" to the package.json that includes a "requiredEnvVars" clause

AND

delete your config file

AND

Add a line to circle.yml to invoke this test

AND

apply these comments to other appengine apps that you modified.

@jmdobry
Copy link
Member

jmdobry commented Aug 23, 2017

Can potentially close this in favor of #463

@ace-n
Copy link
Contributor Author

ace-n commented Aug 29, 2017

This was replaced by #468.

ace-n pushed a commit that referenced this pull request Nov 14, 2022
ace-n pushed a commit that referenced this pull request Nov 14, 2022
* build!: Update library to use Node 12

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
ace-n pushed a commit that referenced this pull request Nov 14, 2022
* build!: Update library to use Node 12

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
ace-n pushed a commit that referenced this pull request Nov 14, 2022
* build!: Update library to use Node 12

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
ace-n pushed a commit that referenced this pull request Nov 15, 2022
* build!: Update library to use Node 12

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
ace-n pushed a commit that referenced this pull request Nov 17, 2022
ahrarmonsur pushed a commit that referenced this pull request Nov 18, 2022
ahrarmonsur pushed a commit that referenced this pull request Nov 18, 2022
NimJay pushed a commit that referenced this pull request Nov 18, 2022
ace-n pushed a commit that referenced this pull request Nov 21, 2022
ace-n pushed a commit that referenced this pull request Nov 21, 2022
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