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

Improve BuiltIn.Evaluate error messages #1798

Merged
merged 2 commits into from
Sep 16, 2014

Conversation

guykisel
Copy link
Contributor

For issue #1795 - improve error messages for empty/nonstring expressions

For issue robotframework#1795 - improve error messages for empty/nonstring expressions
@robotci
Copy link

robotci commented Sep 12, 2014

Admin, can CI be enabled?

return eval(expression, namespace)
except:
except Exception:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pekkaklarck I changed this because an unspecified except: is generally avoided in Python, but I'm not completely sure if it was intentional in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, using except: here and in many other places in Robot code base is intentional. The reason is that Java based exceptions are not based on Exception and thus go through except Exception:. The problem with plain except: is that it catches also SystemExit and KeyboardInterrupt, but this is handled by utils.get_error_message/details that reraises them and that ought to be used everywhere where plain except: is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation. I'll change it back.

@guykisel
Copy link
Contributor Author

Does this work for non-admin users?

@robotci: once

@pekkaklarck
Copy link
Member

@guykisel: Nope, only admins can control CI. Would be too big security risk to allow anyone to run any code there. But we can whitelist you so that your pull-requests ought to be run automatically. Let's see does this work:

@robotci: whitelist user

@robotci
Copy link

robotci commented Sep 16, 2014

Tests passed.

Refer to this link for build results (access rights to CI server needed):
http://robot.radiaatto.ri.fi/job/RobotFramework-pull-requests/3/

@guykisel
Copy link
Contributor Author

@robotci: once

@robotci
Copy link

robotci commented Sep 16, 2014

Tests passed.

Refer to this link for build results (access rights to CI server needed):
http://robot.radiaatto.ri.fi/job/RobotFramework-pull-requests/4/

@pekkaklarck
Copy link
Member

@guykisel: I think you don't need to explicitly tell CI to run tests now that you are whitelisted. All your PRs ought to be run automatically.

Or how was it @jussimalinen? Also, what was the method to avoid running tests if there is no need? We definitely need to list all commands somewhere. CONTRIBUTING.rst is probably the best place once we have written it, and until that listing them in a comment to #1768 is probably the easiest solution.

pekkaklarck added a commit that referenced this pull request Sep 16, 2014
Improve BuiltIn.Evaluate error messages.

Affects also all keywords that use it internally (e.g. `Should Be True`, `Run Keyword If`).
@pekkaklarck pekkaklarck merged commit 04d3853 into robotframework:master Sep 16, 2014
@guykisel guykisel deleted the evaluate-error-msg branch September 16, 2014 20:33
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