-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Core: Fix SnapshotProducer#targetBranch's exception message #7315
Core: Fix SnapshotProducer#targetBranch's exception message #7315
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great catch @zhongyujiang the fix looks good to me. can we add a unit test for this? This bug implies this precondition check is not tested so it would be good to have coverage though.
.as("Invalid branch") | ||
.isInstanceOf(IllegalArgumentException.class) | ||
.hasMessageContaining( | ||
"some-tag is a tag, not a branch. Tags cannot be targets for producing snapshots"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amogh-jahagirdar These two unit tests already test the precondition check, but the exception msg is not validated, so I updated them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the contribution @zhongyujiang !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the nits have been applied
() -> table.newFastAppend().appendFile(FILE_A).toBranch("some-tag").commit()) | ||
.as("Invalid branch") | ||
.isInstanceOf(IllegalArgumentException.class) | ||
.hasMessageContaining( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: hasMessage
would be better here, since we can match the full error msg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Assertions.assertThatThrownBy(() -> table.newFastAppend().appendFile(FILE_A).toBranch(null)) | ||
.as("Invalid branch") | ||
.isInstanceOf(IllegalArgumentException.class) | ||
.hasMessageContaining("Invalid branch name: null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: hasMessage
would be better here, since we can match the full error msg
Merged, thanks @zhongyujiang , and @amogh-jahagirdar , @nastra for reviews |
No description provided.