Skip to content

Add ITComputeTest test for resource name - #4417

Merged
andreamlin merged 7 commits into
googleapis:masterfrom
andreamlin:test_resource_name
Feb 22, 2019
Merged

Add ITComputeTest test for resource name#4417
andreamlin merged 7 commits into
googleapis:masterfrom
andreamlin:test_resource_name

Conversation

@andreamlin

@andreamlinandreamlin commented Jan 29, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@andreamlinandreamlin added the status: blocked Resolving the issue is dependent on other work. label Jan 29, 2019
@andreamlin
andreamlin requested a review from a teamJanuary 29, 2019 20:40
@googlebotgooglebot 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 teamJanuary 29, 2019 20:41
@JustinBeckwithJustinBeckwith added the 🚨 critical P0 critical issue. Requires immediate fix label Feb 7, 2019
@yoshi-automationyoshi-automation removed the 🚨 critical P0 critical issue. Requires immediate fix label Feb 7, 2019
@andreamlinandreamlin removed the status: blocked Resolving the issue is dependent on other work. label Feb 7, 2019
@andreamlin

andreamlin commented Feb 7, 2019

Copy link
Copy Markdown
ContributorAuthor

How do i format the code?

@igorbernstein2

igorbernstein2 commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

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

@andreamlin

andreamlin commented Feb 7, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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-automationyoshi-automation added the 🚨 critical P0 critical issue. Requires immediate fix label Feb 7, 2019
@andreamlin

andreamlin commented Feb 19, 2019

Copy link
Copy Markdown
ContributorAuthor

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

@kolea2

kolea2 commented Feb 20, 2019

Copy link
Copy Markdown
Contributor

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

codecovBot commented Feb 20, 2019•
edited
Loading

Copy link
Copy Markdown

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.

@andreamlinandreamlin mentioned this pull request Feb 20, 2019
Merged
@andreamlin

andreamlin commented Feb 20, 2019

Copy link
Copy Markdown
ContributorAuthor

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

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

fail();

@kolea2kolea2Feb 20, 2019

Copy link
Copy Markdown
Contributor

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?

@andreamlinandreamlinFeb 21, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done.

Comment thread...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'");

@kolea2kolea2Feb 20, 2019

Copy link
Copy Markdown
Contributor

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?

@andreamlinandreamlinFeb 21, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done!

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

@Test
public void testInsertInstance() {

@kolea2kolea2Feb 20, 2019

Copy link
Copy Markdown
Contributor

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?

@andreamlinandreamlinFeb 21, 2019

Copy link
Copy Markdown
ContributorAuthor

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.

@andreamlinandreamlinFeb 21, 2019

Copy link
Copy Markdown
ContributorAuthor

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

andreamlin commented Feb 21, 2019

Copy link
Copy Markdown
ContributorAuthor

Fixed up comments. PTAL

@sduskissduskis added status: blocked Resolving the issue is dependent on other work. and removed 🚨 critical P0 critical issue. Requires immediate fix labels Feb 21, 2019
@andreamlinandreamlin removed the status: blocked Resolving the issue is dependent on other work. label Feb 21, 2019
@yoshi-automationyoshi-automation added the 🚨 critical P0 critical issue. Requires immediate fix label Feb 21, 2019
@andreamlin
andreamlin merged commit 6300b45 into googleapis:masterFeb 22, 2019
@andreamlin
andreamlin deleted the test_resource_name branch February 22, 2019 21:05
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@kolea2kolea2kolea2 left review comments
@sduskissduskissduskis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yesThis human has signed the Contributor License Agreement.🚨 criticalP0 critical issue. Requires immediate fix

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