Skip to content

Conversation

@gryczj
Copy link
Contributor

@gryczj gryczj commented Sep 10, 2024

Description

Fixes #

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

  • I have followed guidelines from CONTRIBUTING.MD and Samples Style Guide
  • Tests pass: npm test (see Testing)
  • Lint pass: npm run lint (see Style)
  • These samples need a new API enabled in testing projects to pass (let us know which ones)
  • These samples need a new/updated env vars in testing projects set to pass (let us know which ones)
  • This pull request is from a branch created directly off of GoogleCloudPlatform/nodejs-docs-samples. Not a fork.
  • This sample adds a new sample directory, and I updated the CODEOWNERS file with the codeowners for this sample
  • This sample adds a new sample directory, and I created GitHub Actions workflow for this sample
  • This sample adds a new Product API, and I updated the Blunderbuss issue/PR auto-assigner with the codeowners for this sample
  • Please merge this PR for me once it is approved

@gryczj gryczj requested review from a team as code owners September 10, 2024 14:46
@gryczj gryczj self-assigned this Sep 10, 2024
@snippet-bot
Copy link

snippet-bot bot commented Sep 10, 2024

Here is the summary of changes.

You are about to add 1 region tag.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@gryczj gryczj added api: compute Issues related to the Compute Engine API. samples Issues that are directly related to samples. kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Sep 10, 2024
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Sep 10, 2024
Copy link
Contributor

@subfuzion subfuzion left a comment

Choose a reason for hiding this comment

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

Need tests to pass. Thanks, @gryczj.

assert.equal(response.specificReservation.count, newVMsNumber);
});

it('should return list of reservations', () => {
Copy link
Contributor

@subfuzion subfuzion Sep 12, 2024

Choose a reason for hiding this comment

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

This test (which wasn't even modified) failed. This why I recommended in your other PR to simplify your tests. There is no need to system test production APIs in your tests (it's hard to get right and it's not a sample's responsibility; the service team exhausitively tests their services). It's only the sample's job to demonstrate a use case for an API and perhaps also demonstrate typical error handling for best practices (or comment that production code should try/catch API calls, etc). Our test code doesn't actually make into the docs, so really they are just there as a guard against broken samples, but samples themselves ought to fully encapsulate whatever is being demonstrated. If the sample throws, it exits with an error code, which is enough to signal we have a broken sample. If you're a developer who wants to list reservations, the sample only needs to demonstrate how to use the API in a contextual, language specific way (vs the REST or gRPC APIs). Code should be fairly copy/pastable. Thanks!

image

https://github.com/GoogleCloudPlatform/nodejs-docs-samples/actions/runs/10821168316/job/30022663732?pr=3844

@subfuzion
Copy link
Contributor

subfuzion commented Sep 12, 2024

@gryczj What's your internal alias so I can contact you on internal chat? Or ping me yourself if you want to discuss this or your other PR: tonypujals@

Copy link
Contributor

@subfuzion subfuzion left a comment

Choose a reason for hiding this comment

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

LGTM, but test flakiness still persists, unfortunately. I'm going to go ahead and merge this because you need to address tests in #3832.

@subfuzion subfuzion merged commit 58745f2 into main Sep 12, 2024
@subfuzion subfuzion deleted the modify-reservation branch September 12, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: compute Issues related to the Compute Engine API. samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants