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

Add error handling for invalid input regexes #182

Merged
merged 1 commit into from
Jun 16, 2018
Merged

Conversation

mtreinish
Copy link
Owner

This commit adds error handling for invalid input regexes on any of the
test selection mechanisms. Now when an invalid regex is encountered an
error message will be printed and then stestr will exit. This should hopefully
make it easier to identify bad user input, like described in #181.

The logic in selection.py needed to be reworked slightly to
differentiate between invalid regex in input filters and the whitelist
file.

@mtreinish mtreinish requested a review from masayukig June 15, 2018 18:54
@coveralls
Copy link

coveralls commented Jun 15, 2018

Coverage Status

Coverage increased (+0.6%) to 65.424% when pulling 6258997 on invalid-regex-error into f798088 on master.

This commit adds error handling for invalid input regexes on any of the
test selection mechanisms. Now when an invalid regex is encountered an
error message will be printed and then stestr will exit.

The logic in selection.py needed to be reworked slightly to
differentiate between invalid regex in input filters and the whitelist
file.
Copy link
Collaborator

@masayukig masayukig left a comment

Choose a reason for hiding this comment

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

LGTM, this change is much better to identify the bad regex for users.

try:
_filters.append(re.compile(f))
except re.error:
print("Invalid regex: %s provided in filters" % f)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking like these error messages should be written to stderr. But it's a separate thing. We already do it in the others.
I'll push a patch to fix it if we should. What do you think?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah using stderr is probably the proper way to handle this. It's a pattern I've used in other places, mostly because I'm lazy. I think you pushing a follow up patch to switch all of those places to use stderr is a good idea.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, cool. I'll push a patch for that later.

@masayukig masayukig merged commit d769fe8 into master Jun 16, 2018
@masayukig masayukig deleted the invalid-regex-error branch June 16, 2018 01:45
masayukig added a commit to masayukig/stestr that referenced this pull request Jun 18, 2018
This commit makes warning and error messages use stderr for
output. Originally, those messages use stdout. However, those messages
should use stderr instead because of the responsibility. This commit
keeps the original exit codes for the backward compatibility.

Related-PR: mtreinish#182
masayukig added a commit to masayukig/stestr that referenced this pull request Jun 18, 2018
This commit makes warning and error messages use stderr for
output. Originally, those messages use stdout. However, those messages
should use stderr instead because of the responsibility. This commit
keeps the original exit codes for the backward compatibility.

Related-PR: mtreinish#182
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.

None yet

3 participants