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

test: add skip for missing dependency, fixture for local tests directory #933

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

lobis
Copy link
Contributor

@lobis lobis commented Nov 25, 2023

While working on #930 I had to run the tests locally and I ran into some problems that other new users may face when running tests:

  • I only installed the dev dependencies (did not install dask dependencies) at first. Tests were failing due to missing dependencies. I added a pytest import skip if dask.distributed is missing.
  • I use the PyCharm IDE and when running a given test function from the IDE it fails as PyCharm tries to run the test function from the location of the code file, instead of running it from the root directory. I added a pytest fixture so that test sample file paths are independant of the directory where the test is run. I didn't cover all instances where a local samples path appears but covered many instances.

@lobis lobis marked this pull request as ready for review November 25, 2023 00:08
@lgray
Copy link
Collaborator

lgray commented Nov 29, 2023

@lobis It may make more sense to include distributed now as part of [dev] or [test] given how much stuff uses it now. Also, a lot of the files you changes are getting deleted in #882. I don't think I'll merge this PR as is.

@lgray
Copy link
Collaborator

lgray commented Dec 6, 2023

Once #882 is merged this PR will need to be redone - but the changes are a good idea.

@lgray
Copy link
Collaborator

lgray commented Dec 14, 2023

@lobis dask distributed is now included in [dev], the addition for pycharm seems nice! Please update this PR at your leisure.

@lobis
Copy link
Contributor Author

lobis commented Dec 14, 2023

@lgray I guess this is ready to merge once the tests pass!

@lgray lgray merged commit 208c0e2 into scikit-hep:master Dec 14, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants