-
Notifications
You must be signed in to change notification settings - Fork 522
Clarify error message when a job uses an input multiple times. #1720
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
Clarify error message when a job uses an input multiple times. #1720
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1720 +/- ##
==========================================
- Coverage 71.6% 69.02% -2.59%
==========================================
Files 175 175
Lines 5364 5359 -5
Branches 328 314 -14
==========================================
- Hits 3841 3699 -142
- Misses 1523 1660 +137
Continue to review full report at Codecov.
|
clairemcginty
left a comment
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.
awesome, thanks for the PR!
| require(!s.contains(key), | ||
| s"There already exists test input for $key, currently " + | ||
| s"registered inputs: ${s.mkString("[", ", ", "]")}") | ||
| require(!s.contains(key), s"Test input $key has already been used once.") |
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.
🎨 "used once" -> "read once"?
while you're at it, would you want to make a matching change to TestOutput? :)
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.
👍
| } | ||
|
|
||
| object JobWitDuplicateInput { | ||
| object JobWithDuplicateInput { |
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.
heh thanks for fixing the typos!
|
Changes made - thanks for the review, @clairemcginty! |
|
thanks for the contribution! looks great 👍 |
Given a Scio job that uses the same input multiple times:
...Scio's
TestDataManagercurrently throws an error in test that reads:There already exists test input for..., which implies that the test itself has multiples of the same test input. This is unclear as the issue is actually with the job itself, not the test.This PR clarifies this error message by changing it to
s"Test input $key has already been used once.", which is hopefully clearer for users that encounter this error.(cc @clairemcginty, @regadas)