Skip to content
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

Make tests more independent #5211

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions MANIFEST.in
Original file line number Diff line number Diff line change
@@ -1 +1,10 @@
include requirements*.txt
include src/toil/server/api_spec/workflow_execution_service.swagger.yaml
include src/toil/test/cwl/colon_test_output_job.yaml
include src/toil/test/cwl/conditional_wf.yaml
include src/toil/test/cwl/mock_mpi/fake_mpi.yml
include src/toil/test/docs/scripts/*
include src/toil/test/utils/ABCWorkflowDebug/sleep.yaml
include src/toil/test/utils/ABCWorkflowDebug/*
Comment on lines +6 to +8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these implicitly scoped by what is in source control? Or will the wildcards potentially include things like editor swap files in the release?

recursive-include src/toil/test/ *.cwl
recursive-include src/toil/test/ *.txt
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ endif
endif

develop: check_venv
python3 setup.py check
pip install -e .$(extras) $(packages)

clean_develop: check_venv
Expand Down
3 changes: 3 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[build-system]
requires = ["setuptools>=64", "setuptools_scm>=8"]
build-backend = "setuptools.build_meta"
4 changes: 1 addition & 3 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,7 @@ def run_setup():
extras_require=extras_require,
package_dir={"": "src"},
packages=find_packages(where="src"),
package_data={
"": ["*.yml", "*.yaml", "cloud-config", "*.cwl"],
},
include_package_data=True,
# Unfortunately, the names of the entry points are hard-coded elsewhere in the code base so
# you can't just change them here. Luckily, most of them are pretty unique strings, and thus
# easy to search for.
Expand Down
23 changes: 22 additions & 1 deletion src/toil/test/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import atexit
import datetime
import logging
import os
Expand All @@ -28,8 +29,10 @@
import zoneinfo
from abc import ABCMeta, abstractmethod
from collections.abc import Generator
from contextlib import contextmanager
from contextlib import ExitStack, contextmanager
from importlib.resources import as_file, files
from inspect import getsource
from pathlib import Path
from shutil import which
from tempfile import mkstemp
from textwrap import dedent
Expand All @@ -52,6 +55,24 @@
logger = logging.getLogger(__name__)


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())
Comment on lines +58 to +73
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



class ToilTest(unittest.TestCase):
"""
A common base class for Toil tests.
Expand Down
Loading
Loading