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

Improve testing #93

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

valeriupredoi
Copy link

Hey @RosalynHatcher hope you don't mind me nosing a bit here 😁 I wrote a quick Github Actions test flow for you (with some explanations in comments) so you can test the installation from PyPi; you can write a similar one to test the install from source, for developers. A few notes from me:

  • tfunctional testing can be improved, be careful that the current test script fails if you don't create the cache dir in a user-accessible location, also it'd be good to put those target dirs outside the git-controlled repo, so you don't create orphan files
  • the nc files that the tests use get modified in-place so that's not good since they're git-controlled

Another side-note, cfchecks --version returns the help docstring but not the version, I think you are not setting it correctly at argparse stage, sorry, am nagging too much 😁

There is also an issue I wanted to flag: it's best you recommend users+developers (the advanced users) to install it in a virt env (either Python or Conda virt env), a la:

conda create -n cfchecker
conda install pip
conda install -c conda-forge udunits2  # this is a runaway dependency that doesn't get installed with pip install -e .
pip install -e .

This is better and more secure than python setup.py install

Anyways, hope this is useful 🍺

@valeriupredoi
Copy link
Author

valeriupredoi commented May 25, 2021

BTW here's the issue with udunits2 as picked up by the GA test without pre-install of it, installing it fixes the actual functionality but it looks like that your help returns a non-zero exit status, check it out, probably related to the issue with the version not being stdouted

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.

1 participant