Skip to content

Conversation

@vkarak
Copy link
Contributor

@vkarak vkarak commented Jul 20, 2021

This PR improves the handling of user programming errors in ReFrame. Generally, I can see three broad categories of user errors:

  1. Standard Python programming errors, such as NameError, AttributeError, ValueError and TypeError.
  2. ReFrame errors that are raised while a test is executed due to programming errors. Examples are the ValueErrors raised by a TypedField.
  3. ReFrame errors that are raised during the test class creation. These are all the errors related to the variable and parameter syntax, as well as all the built-ins.

Errors in (1) and (2) can be tracked back to user test line that triggered them, whereas errors in (3) cannot. So a user programming error can be defined as one of the error types in (1) and if the frame that triggered it is in the "user space." In these cases, we simply print the error message and its source context and continue by skipping the offending test. For the category (3), we can't do that, because we can't differentiate whether a ValueError or AttributeError is actually a user programming error or a bug. So for these type of errors, we raise ReframeSyntaxError, which is treated as a soft error, i.e., print the error message (without source context obviously, but showing the test that is being created) and skip the test. In all cases, the users can use -v to get the full stack trace.

This PR also deactivates and deprecates the --ignore-check-conflicts. It had practically no effect other than not stopping the loading of tests. Currently, if a duplicate test is loaded, it is simply ignored with a warning message.

Fixes #1899.

@pep8speaks
Copy link

pep8speaks commented Jul 20, 2021

Hello @vkarak, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2021-07-30 21:11:41 UTC

@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2021

Codecov Report

Merging #2087 (c54c3f3) into master (f74a51c) will increase coverage by 0.03%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2087      +/-   ##
==========================================
+ Coverage   86.26%   86.30%   +0.03%     
==========================================
  Files          53       53              
  Lines        9347     9352       +5     
==========================================
+ Hits         8063     8071       +8     
+ Misses       1284     1281       -3     
Impacted Files Coverage Δ
reframe/core/warnings.py 100.00% <ø> (ø)
reframe/core/decorators.py 64.48% <70.00%> (-0.33%) ⬇️
reframe/frontend/cli.py 75.95% <75.00%> (-0.05%) ⬇️
reframe/frontend/loader.py 92.12% <92.85%> (-0.44%) ⬇️
reframe/core/exceptions.py 97.27% <100.00%> (+2.64%) ⬆️
reframe/core/meta.py 98.92% <100.00%> (ø)
reframe/core/namespaces.py 95.08% <100.00%> (+0.08%) ⬆️
reframe/core/parameters.py 100.00% <100.00%> (ø)
reframe/core/variables.py 96.10% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f74a51c...c54c3f3. Read the comment docs.

Copy link
Contributor

@ekouts ekouts left a comment

Choose a reason for hiding this comment

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

Lgtm

@teojgo
Copy link
Contributor

teojgo commented Jul 21, 2021

@vakak should the documentation and the reference manual be also updated regarding the deprecation of --ignore-check-conflicts?

@vkarak
Copy link
Contributor Author

vkarak commented Jul 21, 2021

@teojgo Good catch, thanks!

Copy link
Contributor

@victorusu victorusu left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@jjotero jjotero left a comment

Choose a reason for hiding this comment

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

I think we should create 2 issues from this PR then:

  • Making the loader smarter and catching all the potential metaclass errors.
  • Create the rationale in the docs.

@vkarak
Copy link
Contributor Author

vkarak commented Jul 23, 2021

I just realised that I need to update the tips and tricks tutorial about debugging. It even has an implicit reference to the @parameterized_test decorator 😬

@vkarak
Copy link
Contributor Author

vkarak commented Jul 29, 2021

@jjotero

Making the loader smarter and catching all the potential metaclass errors.

What do you mean here? Currently, it catches what it needs to, since metaclasses throw ReframeSyntaxError. Do you mean that it should become smarter in order to allow metaclasses thrown anything?

Create the rationale in the docs.

I think this should be an internal documentation. In the documentation ("Tips & Tricks" tutorial) I have the following now:

Generally, ReFrame will not print the full stack trace for user programming errors and will not block the test loading process. If a test has errors and cannot be loaded, an error message will be printed and the loading of the remaining tests will continue.

@vkarak vkarak merged commit 7afe689 into reframe-hpc:master Jul 30, 2021
@vkarak vkarak deleted the feat/improve-error-messages branch July 30, 2021 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue more user-friendly errors on variable and parameter errors

7 participants