Scheme: test if scheme examples can be opened #2242
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2242 +/- ##
=========================================
Coverage ? 73.01%
=========================================
Files ? 316
Lines ? 55192
Branches ? 0
=========================================
Hits ? 40300
Misses ? 14892
Partials ? 0 |
|
Make sure you add new paths that should be included in distribution to MANIFEST and setup.py (package_data). |
jerneju
changed the title
There was a problem hiding this comment.
While we try to follow most of the lints suggestions, doing so blindly without understanding the consequences is a bad idea. Import order in setup.py matter, as one library (setuptools) might patch the other (distutils) and having a specific order of imports ensures that we get and use the patched methods.
The test looks ok, just try to refactor it a bit to make it easier to add addition workflows in the future. I have included some suggestions in the code.
setup.py
Outdated
| @@ -3,6 +3,7 @@ | |||
| import os | |||
| import sys | |||
| import subprocess | |||
| from distutils.command.build_ext import build_ext | |||
setup.py
Outdated
| @@ -228,7 +229,7 @@ def run(self): | |||
|
|
|||
|
|
|||
|
|
|||
| class build_ext_error(build_ext): | |||
| class BuildExtError(build_ext): | |||
| and also those placed "workflows" subfolder. | ||
| GH-2240 | ||
| """ | ||
| def test(filename): |
There was a problem hiding this comment.
convert this function to a (static) method on the class. name it something along the lines of "assertCanBeLoaded".
Add some error handling code to the test that would catch the error and fail the test with the name of the workflow that could not be loaded. When the tests fails, the name of the problematic workflow will be much more useful than the actual exception (as it will allow us to run and debug the problematic workflow).
| with open(filename, "rb") as f: | ||
| scheme_load(new_scheme, f) | ||
|
|
||
| tutors = workflows.example_workflows() |
There was a problem hiding this comment.
Move the specification of the workflows to be tested outside of the tests. I define them in a constant on a module level that would look like this:
TEST_WORKFLOWS=itertools.chain(
workflows.example_workflows(),
discover_workflows(dirname(__file__))
)
(I assume that you write a function discover_workflows that gets a path to a dir and yields all .ows files it finds by traversing the directory) This would simplify the test to the point where the checks in test() function could be written in the body of the for loop that iterates through the TEST_WORKFLOWS.
| try: | ||
| scheme_load(new_scheme, f) | ||
| except UnknownWidgetDefinition as e: | ||
| log.error("Unknown widget '%s' while loading '%s'.", |
There was a problem hiding this comment.
use self.fail instead of logging and then raising.
| exc_info=True) | ||
| raise | ||
| except Exception: | ||
| log.error("Error occurred while loading '%s'.", |
| GH-2240 | ||
| """ | ||
| for ows_file in TEST_WORKFLOWS: | ||
| self.assertCanBeLoaded(ows_file) |
There was a problem hiding this comment.
as this test is pretty short now, what about moving the content of the assert method back to the test?
|
@astaric: Should I write another test to increase codecov/patch diff hit? |
Issue
#2240
Description of changes
Includes