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

Adding CI #7

Merged
merged 29 commits into from
Jul 23, 2023
Merged

Adding CI #7

merged 29 commits into from
Jul 23, 2023

Conversation

BenediktBurger
Copy link
Member

@BenediktBurger BenediktBurger commented Jul 12, 2023

Tries to setup automatic testing and versioning
Closes #4
closes #6

@BenediktBurger BenediktBurger added the enhancement New feature or request label Jul 12, 2023
@BenediktBurger BenediktBurger marked this pull request as ready for review July 12, 2023 14:42
@BenediktBurger
Copy link
Member Author

Another PR around 1000 lines, but most of it is copied from pymeasure and adjusted accordingly.

It's good, that we set it up: I already encountered one incompatibility with python 3.10 (I use 3.11).

@BenediktBurger
Copy link
Member Author

BenediktBurger commented Jul 13, 2023

Open:

  • Static Tests
  • test coverage
  • lining with ruff (in the beginning parallel to flake8? )

@bilderbuchi
Copy link
Member

lining with ruff (in the beginning parallel to flake8? )

I'd just use ruff, and not flake8 anymore, no sense in duplicating this.
There is a Github Action available for it: https://github.com/chartboost/ruff-action

Also, consider using mamba instead of conda, it should be much faster (https://github.com/mamba-org/setup-micromamba should be the right action)... it's kinda crazy how much time package/env setup is taking in the pymeasure CI jobs.

@bilderbuchi
Copy link
Member

bilderbuchi commented Jul 14, 2023

Great work! I have to go through it later..

Maybe we could exclude the top level init.py (with the versioning code) from coverage. I already excluded the definition files, which follow LECO (leco-protocols and errors), as it is not useful to test these definitions.

I would not ignore the init file, but just specifically ignore that clause for the error condition 👍

except (ImportError, LookupError):  # pragma: no cover

That is more targeted, and the rest of the file remains covered.

For teh leco-protocol, you seem to even give testing instructions (I only briefly skimmed). You don't think this should be tested in pytest? Or does just the coverage checking not work there?

@bilderbuchi
Copy link
Member

Regarding ruff: should we move that extend-select to the config in pyproject.toml, too, or is there a specific reason why that is specified in the CI config?

@BenediktBurger
Copy link
Member Author

I give instructions on how to use these protocol definitions to test an implementation of that protocol.

Testing the protocols has two difficulties:

  • how to test abstract methods?
  • what advantage do we get? Right now the protocol defines the behavior (signature). With tests, the tests define the behavior.

@bilderbuchi
Copy link
Member

bilderbuchi commented Jul 14, 2023

Re the tests/**/__init__.py files: Those should not be added AFAIK. Why was that necessary, what was the problem here?

@BenediktBurger
Copy link
Member Author

Coverage calculated needs them. Otherwise it says 0%...
That's what I tried to say with the commit message.

@BenediktBurger
Copy link
Member Author

Regarding ruff: should we move that extend-select to the config in pyproject.toml, too, or is there a specific reason why that is specified in the CI config?

The reason is, that flake was configured that way I. Pymeasure. I just adapted it.

@bilderbuchi
Copy link
Member

coverage of testing code does not make sense, though, does it? It's the testing code that creates coverage after all?
The only thing that guards against is if some tests are skipped/ignored or inadvertantly not run, but I'm not sure if that's worth it.
I remember pytest docs cautioning against adding init to test folders, but could not quickly find that part.

@bilderbuchi
Copy link
Member

I give instructions on how to use these protocol definitions to test an implementation of that protocol.

Testing the protocols has two difficulties:

* how to test abstract methods?

* what advantage do we get? Right now the protocol defines the behavior (signature). With tests, the tests define the behavior.

OK, fair enough!

@BenediktBurger
Copy link
Member Author

BenediktBurger commented Jul 14, 2023

coverage of testing code does not make sense, though, does it? It's the testing code that creates coverage after all? The only thing that guards against is if some tests are skipped/ignored or inadvertantly not run, but I'm not sure if that's worth it. I remember pytest docs cautioning against adding init to test folders, but could not quickly find that part.

Sorry for not being specific enough: Coverage needs init files in the test directory to calculate the coverage of the main source files...
I did not find the original recommendation to add these, but here is an issue: pytest-dev/pytest-cov#401

EDIT: here the suggestion to add init.py files https://stackoverflow.com/questions/47287721/coverage-py-warning-no-data-was-collected-no-data-collected#52044708

@BenediktBurger
Copy link
Member Author

I figured it out: You have to install the package editable, then you do not need the init files.

@bilderbuchi
Copy link
Member

that's probably just a workaround/hack, but at least the problem's avoided. 👍
It seems from this example that our layout should be achievable without init files, and without going to a src layout, but it will be some time before I get around to play around with this locally.
I guess a src-based layout would in general be preferable because it is quicker to surface path/import problems (which can be hidden otherwise, see pytest docs), but I haven't yet tried that out.

@BenediktBurger
Copy link
Member Author

BenediktBurger commented Jul 14, 2023

I fixed all your points (except the ruff configuration):

  • No init.py in test directories
  • global init.py is covered with "no cover"
  • leco definitions (purely abstract methods or numbers from the leco definition) are not tested
  • ruff configuration from CI also in pyproject.toml?

EDIT: I pushed temporarily (changed in a commit, reversed in the next) a broken message file which was (correctly) shown as broken by mypy. So that works as expected.

Copy link
Member

@bilderbuchi bilderbuchi left a comment

Choose a reason for hiding this comment

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

Looks good, I had some minor remarks.

.github/workflows/pyleco_CI.yml Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
environment.yml Show resolved Hide resolved
@bilderbuchi
Copy link
Member

Thanks for all the work! Feel free to merge!

@BenediktBurger BenediktBurger merged commit 8fe6823 into main Jul 23, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure setuptools for automatic versioning. Setup automated tests
2 participants