Make tests more independent #5211
Conversation
mr-c
force-pushed
the
test_data_location
branch
4 times, most recently
from
4230cab to
65f9e71
Compare
mr-c
force-pushed
the
test_data_location
branch
from
65f9e71 to
fdbe032
Compare
| include src/toil/test/docs/scripts/* | ||
| include src/toil/test/utils/ABCWorkflowDebug/sleep.yaml | ||
| include src/toil/test/utils/ABCWorkflowDebug/* |
There was a problem hiding this comment.
Are these implicitly scoped by what is in source control? Or will the wildcards potentially include things like editor swap files in the release?
| def get_data(filename: str) -> str: | ||
| """Returns an absolute path for a file from this package.""" | ||
| # normalizing path depending on OS or else it will cause problem when joining path | ||
| filename = os.path.normpath(filename) | ||
| filepath = None | ||
| # import ipdb; ipdb.set_trace() | ||
| try: | ||
| file_manager = ExitStack() | ||
| atexit.register(file_manager.close) | ||
| traversable = files("toil") / filename | ||
| filepath = file_manager.enter_context(as_file(traversable)) | ||
| except ModuleNotFoundError: | ||
| pass | ||
| if not filepath or not os.path.isfile(filepath): | ||
| filepath = Path(os.path.dirname(__file__)) / ".." / ".." / ".." / filename | ||
| return str(filepath.resolve()) |
There was a problem hiding this comment.
Should _projectRootPath() and ToilTest.rootDir be removed in favor of this as the One True Way to access test files? (And if this is the One True Way, the docstring should note that so we know nwver to not use this function.)
Should this maybe be a member of ToilTest so it doesn't need a separate import and more sensibly replaces self.rootDir references?
Also, if the source tree is laid out so that a CWL file references another file nearby, and I get_data("whatever.cwl"), does get_data() promise that the path it returns will have those nearby files from the source tree still available, without explicitly being asked for them?
There was a problem hiding this comment.
Also, are we sure that the atexit trickery to defeat the context manager is worth it? I don't see many tests using more than 2 get_data() calls, so even if it was a context manager you had to be in for each file, the nesting wouldn't be that bad. If we had a way to say with get_datas(path1, path2, path3) as (file1, file2, file3): for any number of arguments we could limit the nesting without fighting the library.
Also fixes #3996
Changelog Entry
To be copied to the draft changelog by merger:
Reviewer Checklist
issues/XXXX-fix-the-thingin the Toil repo, or from an external repo.camelCasethat want to be insnake_case.docs/running/{cliOptions,cwl,wdl}.rstMerger Checklist