Remove some local-dev test detritus #3393
Conversation
|
|
||
| with open(str(pfile), 'r') as opfile: | ||
| assert (opfile.readlines()[0] == 'Hello') | ||
| with open(pfile) as opfile: |
There was a problem hiding this comment.
this is changing what's being tested: before, it tested that whatever a File returned as its str could be opened. Now it's testing that whatever comes back as its fspath can be opened.
That str implementation is a bit horrible (it raises an exception for File objects which don't have a local representation) but I think it's still needed as part of usability: for example, people writing bash_apps would traditionally write f"cat {myfile}" when forming their bash command lines rather than `f"cat {os.fspath(myfile)}"
So maybe this should be testing both behaviours?
|
|
||
| # this should run and create a file named after path_x | ||
| no_checkpoint_stdout_app(stdout=path_x).result() | ||
| assert os.path.exists(path_x) | ||
| no_checkpoint_stdout_app(stdout=str(path_x)).result() |
There was a problem hiding this comment.
Recent work should have this still work with path_x being an os.PathLike, which I think is the case here - for example, see the monitoring tests in parsl/tests/test_monitoring/test_stdouterr.py (although those tests only check that the path goes through to monitoring correctly - not that the path really is used by the executing app)
There was a problem hiding this comment.
(i.e. this str should be unnecessary)
(and maybe should be os.fspath)
(at which point you might get a bytes)
(and have to deal with that)
| path_x = "test.memo.stdout.x" | ||
| if os.path.exists(path_x): | ||
| os.remove(path_x) | ||
| no_checkpoint_stdout_app_ignore_args(stdout=str(path_x)).result() |
There was a problem hiding this comment.
see other comment about hopefully-redundant str
|
@khk-globus these seem good. I made some comments you might want to address before merge if you have time/capacity, but I will label approved so you can also merge without. |
yadudoc
force-pushed
the
round_of_filesystem_test_detritus_removal
branch
from
4f8d545
to
a15e039
Compare
Type of change