Skip to content

Add ITComputeTest test for resource name #4417

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
Merged
andreamlin merged 7 commits into from
Feb 22, 2019
Merged

Add ITComputeTest test for resource name #4417

andreamlin merged 7 commits into from
Feb 22, 2019

Conversation

@andreamlin
Copy link
Contributor

@andreamlin andreamlin commented Jan 29, 2019 •
edited
Loading

For testing #4416.

This test won't pass until artman is updated with googleapis/gapic-generator#2529 and the clients are refreshed. Edit: blockers are gone

@andreamlin andreamlin added the status: blocked Resolving the issue is dependent on other work. label Jan 29, 2019
@andreamlin andreamlin requested a review from a team as a code owner January 29, 2019 20:40
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 29, 2019
@andreamlin andreamlin removed the request for review from a team January 29, 2019 20:41
@JustinBeckwith JustinBeckwith added the 🚨 This issue needs some love. label Feb 7, 2019
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Feb 7, 2019
@andreamlin andreamlin requested a review from kolea2 February 7, 2019 18:10
@andreamlin andreamlin removed the status: blocked Resolving the issue is dependent on other work. label Feb 7, 2019
@andreamlin
Copy link
Contributor Author

andreamlin commented Feb 7, 2019

How do i format the code?

@igorbernstein2
Copy link

igorbernstein2 commented Feb 7, 2019

I think its mvn com.coveo:fmt-maven-plugin:2.8:format

@andreamlin
Copy link
Contributor Author

andreamlin commented Feb 7, 2019 •
edited
Loading

@igorbernstein2 No luck, the tool returns success without changing any files.

I've also run synthtool just for the formatting and that hasn't changed anything.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 7, 2019
@andreamlin
Copy link
Contributor Author

andreamlin commented Feb 19, 2019

@kolea2 Is there anything else I can do to fix the formatting on the diff?

@kolea2
Copy link
Contributor

kolea2 commented Feb 20, 2019

Hey @andreamlin! I locally ran 'mvn com.coveo:fmt-maven-plugin:format' on your branch and it reformatted the file for me - looks like there are some unused imports and it reformatted a comment. Let me know if you need help getting it to work or I can try to push my changes as well.

@codecov
Copy link

codecov bot commented Feb 20, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@e67981a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4417   +/-   ##
=========================================
  Coverage          ?   48.52%           
  Complexity        ?    20742           
=========================================
  Files             ?     2077           
  Lines             ?   203081           
  Branches          ?    23294           
=========================================
  Hits              ?    98547           
  Misses            ?    96458           
  Partials          ?     8076

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 e67981a...75fe066. Read the comment docs.

@andreamlin andreamlin mentioned this pull request Feb 20, 2019
Merged
@andreamlin
Copy link
Contributor Author

andreamlin commented Feb 20, 2019

@kolea2 Thanks for the fix! Would you mind reviewing?

kolea2
kolea2 reviewed Feb 20, 2019
View reviewed changes
...clients/google-cloud-compute/src/test/java/com/google/cloud/compute/v1/it/ITComputeTest.java Outdated
return;
}

fail();
Copy link
Contributor

@kolea2 kolea2 Feb 20, 2019

Choose a reason for hiding this comment

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

Can we add a message explaining why it failed?

Copy link
Contributor Author

@andreamlin andreamlin Feb 21, 2019

Choose a reason for hiding this comment

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

Done.

...clients/google-cloud-compute/src/test/java/com/google/cloud/compute/v1/it/ITComputeTest.java Outdated
// problem.
assertThat(e.getMessage()).contains("Bad Request");
assertThat(e.getCause().getMessage())
.doesNotContain("Invalid value for field 'resource.machineType'");
Copy link
Contributor

@kolea2 kolea2 Feb 20, 2019

Choose a reason for hiding this comment

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

My only concern here is that if the error message ever changes, this would still pass. Can we instead assert what the error message actually is?

Copy link
Contributor Author

@andreamlin andreamlin Feb 21, 2019

Choose a reason for hiding this comment

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

Done!

...clients/google-cloud-compute/src/test/java/com/google/cloud/compute/v1/it/ITComputeTest.java
}

@Test
public void testInsertInstance() {
Copy link
Contributor

@kolea2 kolea2 Feb 20, 2019

Choose a reason for hiding this comment

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

Do we have a test that asserts a successfully inserted instance?

Copy link
Contributor Author

@andreamlin andreamlin Feb 21, 2019

Choose a reason for hiding this comment

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

It'll be in https://github.com/googleapis/google-cloud-java/pull/4420/files#diff-72d767a100f949c4606b04bb6820a8feR248. That is blocked on an LRO implementation tho.

Copy link
Contributor Author

@andreamlin andreamlin Feb 21, 2019

Choose a reason for hiding this comment

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

So this should be fine for now, and a later PR will test inserting an instance.

@andreamlin
Copy link
Contributor Author

andreamlin commented Feb 21, 2019

Fixed up comments. PTAL

@sduskis sduskis added status: blocked Resolving the issue is dependent on other work. and removed 🚨 This issue needs some love. labels Feb 21, 2019
@andreamlin andreamlin removed the status: blocked Resolving the issue is dependent on other work. label Feb 21, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 21, 2019
@andreamlin andreamlin merged commit 6300b45 into googleapis:master Feb 22, 2019
@andreamlin andreamlin deleted the test_resource_name branch February 22, 2019 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@kolea2 kolea2 kolea2 left review comments

@sduskis sduskis sduskis approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement. 🚨 This issue needs some love.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@andreamlin @igorbernstein2 @kolea2 @sduskis @JustinBeckwith @googlebot @yoshi-automation