Skip to content
This repository was archived by the owner on Dec 3, 2023. It is now read-only.

tests: clean up tests for IAM condition support - #224

Merged
frankyn merged 3 commits into
googleapis:masterfrom
athakor:core-169
May 28, 2020
Merged

tests: clean up tests for IAM condition support #224
frankyn merged 3 commits into
googleapis:masterfrom
athakor:core-169

Conversation

@athakor

@athakorathakor commented May 27, 2020

Copy link
Copy Markdown
Contributor

Fixes#169

@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label May 27, 2020
@athakor
athakor requested a review from frankynMay 27, 2020 14:22
@codecov

codecovBot commented May 27, 2020•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #224 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #224 +/- ##
============================================
+ Coverage 67.61% 67.62% +0.01% - Complexity 379 384 +5 
============================================
Files 36 36 Lines 1976 1986 +10 Branches 262 269 +7 ============================================
+ Hits 1336 1343 +7 - Misses 532 535 +3 
Partials 108 108
Impacted FilesCoverage ΔComplexity Δ
...re/src/main/java/com/google/cloud/RetryHelper.java50.00% <0.00%> (-3.85%)2.00% <0.00%> (ø%)
...d-core/src/main/java/com/google/cloud/Binding.java100.00% <0.00%> (ø)2.00% <0.00%> (ø%)
...core/src/main/java/com/google/cloud/Condition.java100.00% <0.00%> (ø)2.00% <0.00%> (ø%)
...ud-core/src/main/java/com/google/cloud/Policy.java92.15% <0.00%> (+0.03%)20.00% <0.00%> (ø%)
...src/main/java/com/google/cloud/ServiceOptions.java43.87% <0.00%> (+0.04%)33.00% <0.00%> (+1.00%)
...a/com/google/cloud/testing/BaseEmulatorHelper.java31.42% <0.00%> (+0.21%)5.00% <0.00%> (ø%)
.../com/google/cloud/MonitoredResourceDescriptor.java84.03% <0.00%> (+0.27%)24.00% <0.00%> (+4.00%)

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 e86dbe7...0d695bc. Read the comment docs.

@athakor

athakor commented May 27, 2020

Copy link
Copy Markdown
ContributorAuthor

@frankyn@netdpb PTAL when you have a moment

netdpb
netdpb suggested changes May 27, 2020
View reviewed changes
Comment threadgoogle-cloud-core/src/test/java/com/google/cloud/PolicyV3Test.java Outdated
Binding.newBuilder().setRole(VIEWER).setMembers(MEMBERS_LIST_1).build();
private static final Binding EDITOR_BINDING =
Binding.newBuilder().setRole(EDITOR).setMembers(MEMBERS_LIST_2).build();
private static final List<Binding> BINDINGS_NO_CONDITIONS =

@netdpbnetdpbMay 27, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
privatestaticfinalList<Binding> BINDINGS_NO_CONDITIONS =
privatestaticfinalImmutableList<Binding> BINDINGS_NO_CONDITIONS =

ImmutableList and friends are especially useful as parameter, method, field, and local variable types, so that users know they are immutable. (For example, the mutation methods are all deprecated, so IDEs will warn about their use.) Please do this throughout the file.

@athakorathakorMay 28, 2020

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 threadgoogle-cloud-core/src/test/java/com/google/cloud/PolicyV3Test.java Outdated
Comment on lines +63 to +64
ImmutableList.copyOf(BINDINGS_NO_CONDITIONS)
.of(
Binding.newBuilder()
.setRole(VIEWER)
.setMembers(MEMBERS_LIST_1)
.setCondition(
Condition.newBuilder()
.setTitle("Condition")
.setDescription("Condition")
.setExpression("Expr")
.build())
.build(),
Binding.newBuilder().setRole(EDITOR).setMembers(MEMBERS_LIST_2).build());
.of(VIEWER_BINDING.toBuilder().setCondition(CONDITION).build(), EDITOR_BINDING);

@netdpbnetdpbMay 27, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ImmutableList.of() is a static method. Here you're calling a static method on an instance (returned by ImmutableList.copyOf()), which basically throws the instance away.

I think you can delete line 63 and just do ImmutableList.of(VIEWER_BINDING.toBuilder()…, EDITOR_BINDING).

@athakorathakorMay 28, 2020

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

@athakor

athakor commented May 28, 2020

Copy link
Copy Markdown
ContributorAuthor

@netdpb PTAL

@athakorathakor added the automerge Merge the pull request once unit tests and other checks pass. label May 28, 2020
@frankyn
frankyn merged commit db345f4 into googleapis:masterMay 28, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@frankynfrankynfrankyn approved these changes
@netdpbnetdpbnetdpb approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

automergeMerge the pull request once unit tests and other checks pass.cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clean up tests for IAM Condition Support

4 participants

@athakor@frankyn@netdpb@googlebot