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

refactor: remove functions_concepts_error_object #1484

Merged
merged 17 commits into from
Oct 28, 2019
Merged

refactor: remove functions_concepts_error_object #1484

merged 17 commits into from
Oct 28, 2019

Conversation

grant
Copy link
Contributor

@grant grant commented Sep 16, 2019

Removes unused and not useful sample.

@grant grant requested a review from ace-n September 16, 2019 14:54
@grant grant self-assigned this Sep 16, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 16, 2019
@grant
Copy link
Contributor Author

grant commented Sep 18, 2019

@ace-n Can you review this assuming the tests pass?

ace-n
ace-n suggested changes Sep 18, 2019
View reviewed changes
functions/concepts/test/index.test.js Outdated
@@ -15,21 +15,7 @@

'use strict';

const sinon = require('sinon');
const assert = require('assert');
const tools = require('@google-cloud/nodejs-repo-tools');
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 remove this file, or should we add a (simple) test for executionCount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal of this PR is not to remove this test file, just to remove the functions_concepts_error_object region tag.

I'm not sure why the other functions in this file don't have tests, but that's a separate issue.

@grant
Copy link
Contributor Author

grant commented Sep 18, 2019

I can delete the test file upon request, but I think it's fine to leave it blank for the other functions that should be tested.

@grant grant added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 20, 2019
@grant
Copy link
Contributor Author

grant commented Sep 20, 2019

@ace-n WDYT about this PR?

@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 20, 2019
@ace-n
Copy link
Contributor

ace-n commented Sep 20, 2019

Apologies for the delay. I'd say go ahead and delete the test file.

@grant grant force-pushed the grant-functions_concepts_error_object branch from 2e67ed3 to 91e0821 Compare September 23, 2019 12:58
@grant grant force-pushed the grant-functions_concepts_error_object branch from 91e0821 to b2ea682 Compare September 23, 2019 13:00
@grant
Copy link
Contributor Author

grant commented Sep 23, 2019

Pardon the noise (bad push).

Removed the test file. PTAL.

@grant
Copy link
Contributor Author

grant commented Sep 23, 2019

Can be merged when tests pass.

@grant grant added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 1, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 1, 2019
@grant
Copy link
Contributor Author

grant commented Oct 1, 2019

Not sure why the PR is out of date w.r.t. master twice.

@grant
Copy link
Contributor Author

grant commented Oct 11, 2019

@ace-n Can you look to why the functions/concepts test is failing? I really think there is a bug with the testing infrastructure rather than this PR.

@ace-n
Copy link
Contributor

ace-n commented Oct 11, 2019 via email

@grant
Copy link
Contributor Author

grant commented Oct 11, 2019

Well, this is one reason to not remove the file...

> nodejs-docs-samples-functions-concepts@0.0.1 test /tmpfs/src/github/nodejs-docs-samples/functions/concepts
> mocha test/*.test.js --timeout=20000

Error: No test files found: "test/*.test.js"
npm ERR! Test failed.  See above for more details.


[ID: 3319830] Build finished after 72 secs, exit value: 1

Can we just proceed to remove the sample/region tag functions_concepts_error_object without removing the file?

Do you want me to remove this file too?:
https://github.com/GoogleCloudPlatform/nodejs-docs-samples/blob/master/.kokoro/functions/concepts.cfg

@fhinkel
Copy link
Contributor

fhinkel commented Oct 14, 2019

I'd prefer if we remove the empty test file.

@grant
Copy link
Contributor Author

grant commented Oct 24, 2019 •
edited
Loading

You'll have to remove the functions/concepts test configs a) internally
(from Kokoro)

@ace-n @fhinkel Can you point me to a doc that guides me on how to locate and remove the test configs from Kokoro? I believe I have done so in the .kokoro folder already.

@fhinkel
Copy link
Contributor

fhinkel commented Oct 25, 2019

Our codelab explains it very well: go/kokoro-codelab

@fhinkel fhinkel added the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 27, 2019
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 27, 2019
@grant grant added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2019
@grant
Copy link
Contributor Author

grant commented Oct 28, 2019

Kokoro config removed with cl/276701784.

@grant
Copy link
Contributor Author

grant commented Oct 28, 2019

Approved. Tests pass. Merging.

@grant grant merged commit 1925c3d into master Oct 28, 2019
@grant grant deleted the grant-functions_concepts_error_object branch October 28, 2019 17:48
@snyk-bot snyk-bot mentioned this pull request Jan 10, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ace-n ace-n ace-n approved these changes

Assignees

@grant grant

Labels
cla: yes This human has signed the Contributor License Agreement. kokoro:force-run Add this label to force Kokoro to re-run the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@grant @ace-n @fhinkel @googlebot @kokoro-team