-
Notifications
You must be signed in to change notification settings - Fork 117
Print filename if a test file contains syntax errors #308
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
Conversation
* Raise a `ReframeSyntaxError` with a descriptive message if parsing of a `RegressionTest` fails. * Create the corresponding unittest.
reframe/frontend/loader.py
Outdated
| except SyntaxError as e: | ||
| message = ('\nRegression check syntax error:\n{0}Line: {1}\n' | ||
| 'File: {2}'.format(e.text, e.lineno, filename)) | ||
| raise ReframeSyntaxError(message) |
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.
I don't like very much reraising a real SyntaxError as ReframeSyntaxError. The purpose of the latter is just to denote errors in the usage of the decorators. I agree the name is not very accurate, but I couldn't come up with a better name.
The problem here is firstly conceptual: ReframeSyntaxErrors are ReframeErrors, but a real SyntaxError can never be a ReframeError. Secondly, reraising it differently may hide useful information from real problems.
The solution to this problem is really simple! Just pass a filename argument to parse(). Have a look here.
unittests/test_loader.py
Outdated
| def test_load_invalid_sytax(self): | ||
| self.assertRaises(ReframeSyntaxError, self.loader.load_from_file, | ||
| 'unittests/resources/checks_unlisted/' | ||
| 'invalid_syntax_check.py') |
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.
Here you should check that the filename attribute of the raised SyntaxError is set properly.
reframe/frontend/loader.py
Outdated
| import reframe.utility as util | ||
| from reframe.core.exceptions import NameConflictError, RegressionTestLoadError | ||
| from reframe.core.exceptions import (NameConflictError, | ||
| RegressionTestLoadError) |
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.
Do not split the line.
| with self.assertRaises(SyntaxError) as e: | ||
| self.loader.load_from_file(invalid_check) | ||
|
|
||
| self.assertEqual(e.exception.filename, invalid_check) |
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.
I know that e is visible outside the with statement, but does it make sense to have this assertion outside the with block?
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.
If you put the assertion inside the with block, it is not reachable since the with block exits once the exception is thrown.
|
@jenkins-cscs retry dom |
Raise a
ReframeSyntaxErrorwith a descriptive message if parsing ofa
RegressionTestfails.Create the corresponding unittest
Fixes #267