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
DM-39672: make it possible to force failures in mock pipelines on the command-line #247
Conversation
ca13aff
to
1ed856e
Compare
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.
Looks good
when mocking execution. This is a colon-separated 3-tuple, | ||
where the first entry the task label, the second the | ||
fully-qualified exception type (empty for ValueError, and the | ||
third a string (usually quoted) of the form passed to --where, |
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.
"usually quoted" may be a bit confusing. I guess quoting is needed for shell only, not for the Python code that parses it?
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.
Yes, just for the shell. Reworded to that effect.
return result | ||
for entry in value: | ||
try: | ||
task_label, error_type_name, where = entry.split(":") |
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.
One more comment - what if where
part also contains a colon (e.g. for slicing)?
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.
Good catch. I've updated this to
task_label, error_type_name, *where_terms = entry.split(":")
where = ":".join(where_terms)
to guard against that.
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.
entry.split(":", 2)
is probably faster 🙂
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.
Neat, I didn't know about that argument.
1ed856e
to
3866086
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #247 +/- ##
==========================================
- Coverage 85.69% 85.53% -0.16%
==========================================
Files 47 47
Lines 4215 4231 +16
Branches 724 726 +2
==========================================
+ Hits 3612 3619 +7
- Misses 451 458 +7
- Partials 152 154 +2
☔ View full report in Codecov by Sentry. |
3866086
to
8a44210
Compare
Checklist
doc/changes