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.

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

chore: add increment API #308

Merged
merged 2 commits into from
May 24, 2023
Merged

chore: add increment API #308

merged 2 commits into from
May 24, 2023

Conversation

poppoerika
Copy link
Contributor

Added increment API along with tests to check:

  • if increments from 0 to expected amount with string field
  • if increments from 0 to expected amount with bytes field
  • if increments with setting and resetting field

Closes #304

@poppoerika poppoerika changed the title chore: add increment API chore: add increment API May 22, 2023
Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

🙈 other than I would like a test that covers a negative amount. Also, CI is red.

And I'm not really well versed with our go sdk so you should get Ellery and/or Pete to do final sign-off.

@@ -531,4 +531,167 @@ var _ = Describe("Scalar methods", func() {
).To(BeAssignableToTypeOf(&DecreaseTtlMiss{}))
})
})

Describe("Increment", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

you should add a test that covers a negative value for amount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have it here for a string field and here for a bytes field.

Is it better if I take them out and make a separate test for a negative amount for easy readability?

pgautier404
pgautier404 previously approved these changes May 24, 2023
Copy link
Contributor

@pgautier404 pgautier404 left a comment

Choose a reason for hiding this comment

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

I'm happy with this when @cprice404 is, but we need to figure out these test failures. If we've switched to V1 tokens for testing, I would expect these to fail and the tests need to be modified. Using legacy tokens, the credential provider's getAuthToken method returns the auth token it was originally constructed with. For V1 tokens, however, getAuthToken returns an api token extracted from the original, so they are no longer expected to match as the tests assert.

@poppoerika poppoerika merged commit a72802c into main May 24, 2023
@poppoerika poppoerika deleted the chore/add-increment branch May 24, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Increment
3 participants