-
Notifications
You must be signed in to change notification settings - Fork 87
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
Update graph tests to always use tmpfile dir #649
Conversation
Codecov Report
@@ Coverage Diff @@
## master #649 +/- ##
=======================================
Coverage 98.97% 98.97%
=======================================
Files 136 136
Lines 4694 4696 +2
=======================================
+ Hits 4646 4648 +2
Misses 48 48
Continue to review full report at Codecov.
|
evalml/pipelines/graphs.py
Outdated
@@ -36,6 +36,8 @@ def make_pipeline_graph(component_list, graph_name, filepath=None): | |||
f = open(filepath, 'w') | |||
f.close() | |||
except IOError: | |||
if os.path.exists(filepath): | |||
os.remove(filepath) |
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.
Codecov is yelling at me about this line... I'll add a mocking unit test!
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.
@jeremyliweishih does the logic here make sense to you? Would be good to have a sanity 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.
Yes it does! tmpdir
might be redundant with this but doesn't hurt!
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.
Actually removing tmpr
may fix codecov?
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.
Yeah I don't think this is necessary, deleting
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.
Looks good other than codecov!
054ad8d
to
3c07623
Compare
except IOError: | ||
raise ValueError(('Specified parent directory does not exist: {}'.format(filepath))) | ||
except (IOError, FileNotFoundError): | ||
raise ValueError(('Specified filepath is not writeable: {}'.format(filepath))) |
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.
So here's what I'm thinking. If the open()
succeeded, that will have created an empty file, but that's fine because the rest of the code will write to that location. And if the open()
failed, there's nothing to clean up, so we don't need to call os.remove(filepath)
3c07623
to
bcbe015
Compare
bcbe015
to
e6cbe4d
Compare
Also clean up files if existence test fails.
Fixes #458