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

services/horizon: Test full coverage of operation type switches #3560

Merged
merged 3 commits into from
Apr 26, 2021

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Apr 21, 2021

Part of #3430

I incorporated new tests, which use xdr's operationTypeMap (now exported as OperationTypeToStringMap) to go over all possible operations, even when we incorporate new ones.

The idea is that, from now on, every time we add a new XDR operation (i.e. through a CAP), the developer gets guided on what needs to be modified based on the failing tests.

The tests I added can be considered finicky, but it's hard to do it generically in a robust way.

@2opremio 2opremio requested a review from a team April 21, 2021 15:54
@2opremio 2opremio changed the title Test full coverage of operation type switches services/horizon: Test full coverage of operation type switches Apr 22, 2021
Copy link
Contributor

@bartekn bartekn left a comment

Choose a reason for hiding this comment

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

Awesome!

// make sure the check works for an unreasonable operation type
_, err := UnmarshalOperation(200000, []byte{})
assert.Error(t, err)
assert.Equal(t, mistmatchErr, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, there's EqualError that can be useful here.

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 didn't know, thanks! However, the error is currently identical, so I think it's fine.

Copy link
Contributor

@bartekn bartekn Apr 26, 2021

Choose a reason for hiding this comment

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

Yeah, I mean, you can merge Error and Equal intro EqualError but the current is fine obviously.

@2opremio 2opremio force-pushed the 3430-test-coverage-of-op-switches branch from f09e5d4 to 255f360 Compare April 26, 2021 17:04
@2opremio 2opremio force-pushed the 3430-test-coverage-of-op-switches branch from 255f360 to c111c4e Compare April 26, 2021 17:52
@2opremio 2opremio force-pushed the 3430-test-coverage-of-op-switches branch from c111c4e to 04038a2 Compare April 26, 2021 18:23
@2opremio 2opremio merged commit 0d2f101 into stellar:master Apr 26, 2021
@2opremio 2opremio deleted the 3430-test-coverage-of-op-switches branch April 26, 2021 21:17
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.

3 participants