Common base class for Parameters,Calculations,Visibilities

[gerw]

This patch addresses #122. It also provides some simple tests for these classes.

Please let me know if you have a better name for the base class.

[gerw]

I think pytest fails due to a problem with permissions, see https://github.com/MishaKav/pytest-coverage-comment/issues/30.

It might be fixed by adding

permissions:
  pull-requests: write

to .github/workflows/pytest.yml? Or does this problem only happens for pull request from forks?

[Bouni]

Strange, for me it worked when I re-activated this feature 🤔

[Bouni]

@gerw please push a CI trigger commit to test #134

[github-actions[bot]]

Coverage Report

File	Stmts	Miss	Cover	Missing
luxtronik
__init__.py	134	106	21%	38–54, 61–66, 69–73, 79, 83, 102–113, 116–119, 122–142, 145–160, 163–180, 183–198, 202–204, 208–209, 213–214
__main__.py	21	21	0%	2–48
calculations.py	32	23	28%	43, 307, 311–319, 324–336, 340–341
datatypes.py	235	13	94%	37, 42, 47, 52, 57, 72–75, 80–83, 92
discover.py	43	34	21%	25–77
parameters.py	50	39	22%	33–36, 1166, 1170–1178, 1186–1206, 1210–1211, 1215–1222
visibilities.py	32	23	28%	13, 372, 376–384, 389–401, 405–406
luxtronik/scripts
dump_changes.py	44	44	0%	5–93
dump_luxtronik.py	28	28	0%	5–64
TOTAL	627	331	47%

Tests	Skipped	Failures	Errors	Time
102	4 :zzz:	0 :x:	0 :fire:	0.999s :stopwatch:

[gerw]

@Bouni: Just triggering the CI did not help.

After I rebased to main, it worked.

[gerw]

I just realized that pytest in the CI did not recognized my new test files. Does anybody know why?

[kbabioch]

I just realized that pytest in the CI did not recognized my new test files. Does anybody know why?

No, don't quite understand this (yet). Just checked out your branch locally and invoked the same command on my machine that is executed in the pipeline. For me it looks like this:

PYTHONPATH=. pytest --junitxml=pytest.xml --cov-report=term-missing:skip-covered --cov=luxtronik tests/ | tee pytest-coverage.txt
============================= test session starts ==============================
platform linux -- Python 3.11.3, pytest-7.4.0, pluggy-1.0.0
rootdir: /home/kbabioch/alpha-innotec-hacking/python-luxtronik
plugins: cov-4.1.0, anyio-3.7.0
collected 110 items

tests/test_calculations.py .                                             [  0%]
tests/test_datatypes.py ...ssss......................................... [ 44%]
......................................................                   [ 93%]
tests/test_parameters.py ......                                          [ 99%]
tests/test_visibilities.py .                                             [100%]

- generated xml file: /home/kbabioch/alpha-innotec-hacking/python-luxtronik/pytest.xml -

---------- coverage: platform linux, python 3.11.3-final-0 -----------
Name                                  Stmts   Miss  Cover   Missing
-------------------------------------------------------------------
luxtronik/__init__.py                   134    106    21%   38-54, 61-66, 69-73, 79, 83, 102-113, 116-119, 122-142, 145-160, 163-180, 183-198, 202-204, 208-209, 213-214
luxtronik/__main__.py                    21     21     0%   2-48
luxtronik/datatypes.py                  235     12    95%   37, 42, 47, 57, 72-75, 80-83, 92
luxtronik/discover.py                    43     34    21%   25-77
luxtronik/scripts/dump_changes.py        44     44     0%   5-93
luxtronik/scripts/dump_luxtronik.py      28     28     0%   5-64
-------------------------------------------------------------------
TOTAL                                   588    245    58%

6 files skipped due to complete coverage.

======================== 106 passed, 4 skipped in 0.34s ========================

So the new test classes have been picked up. Not sure why this is not the case here in this pull request. Maybe some kind of caching? Re-triggering the job did not help, though, will have to investigate further.

[kbabioch]

Overall the changes look fine to me. Unit tests also seem to work fine (locally), so in case you want to go ahead with the merge, it's fine with me. The unit test issue needs to be addressed eventually, though.