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

Common base class for Parameters,Calculations,Visibilities #129

Merged
merged 5 commits into from
Jul 23, 2023

Conversation

gerw
Copy link
Collaborator

@gerw gerw commented Jul 21, 2023

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 gerw linked an issue Jul 22, 2023 that may be closed by this pull request
@gerw
Copy link
Collaborator Author

gerw commented Jul 22, 2023

I think pytest fails due to a problem with permissions, see MishaKav/pytest-coverage-comment#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
Copy link
Owner

Bouni commented Jul 22, 2023

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

@Bouni
Copy link
Owner

Bouni commented Jul 22, 2023

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

@github-actions
Copy link

github-actions bot commented Jul 22, 2023

Coverage

Coverage Report
FileStmtsMissCoverMissing
luxtronik
   __init__.py13410621%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__.py21210%2–48
   calculations.py322328%43, 307, 311–319, 324–336, 340–341
   datatypes.py2351394%37, 42, 47, 52, 57, 72–75, 80–83, 92
   discover.py433421%25–77
   parameters.py503922%33–36, 1166, 1170–1178, 1186–1206, 1210–1211, 1215–1222
   visibilities.py322328%13, 372, 376–384, 389–401, 405–406
luxtronik/scripts
   dump_changes.py44440%5–93
   dump_luxtronik.py28280%5–64
TOTAL62733147% 

Tests Skipped Failures Errors Time
102 4 💤 0 ❌ 0 🔥 0.999s ⏱️

@gerw
Copy link
Collaborator Author

gerw commented Jul 22, 2023

@Bouni: Just triggering the CI did not help.

After I rebased to main, it worked.

luxtronik/data_vector.py Show resolved Hide resolved
luxtronik/data_vector.py Outdated Show resolved Hide resolved
luxtronik/data_vector.py Outdated Show resolved Hide resolved
tests/test_visibilities.py Show resolved Hide resolved
@gerw
Copy link
Collaborator Author

gerw commented Jul 22, 2023

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

@kbabioch
Copy link
Collaborator

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
Copy link
Collaborator

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.

@gerw gerw merged commit 300a08a into Bouni:main Jul 23, 2023
@gerw gerw deleted the common_base_class branch July 23, 2023 20:31
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.

Create base class for Calculations, Parameters, Visibilities
3 participants