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.

Sign up for GitHub

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

github-actions: add codecov upload #1

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

github-actions: add codecov upload #1

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7614f30
github-actions: add codecov upload
miri64 Jul 7, 2020
41b4df4
fixup! github-actions: add codecov upload
miri64 Jan 22, 2021
ef0f6d4
setup.cfg: combine coverage results
miri64 Sep 24, 2021
b0ddb50
setup.cfg / coverage: exclude NotImplementedErrors et al.
miri64 Sep 24, 2021
89d86af
squash! setup.cfg: combine coverage results
miri64 Sep 24, 2021
f1d392c
fixup! setup.cfg / coverage: exclude NotImplementedErrors et al.
miri64 Sep 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .github/workflows/tox.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,16 @@ jobs:
- name: Test with tox
run: |
pyenv="py$(echo "${{ matrix.python-version }}" | tr -d '.')"
tox -e ${pyenv}-test,${pyenv}-rapidjson,flake8,lint
tox -e ${pyenv}-test,flake8,lint
tox -e ${pyenv}-rapidjson -- --cov-append
- name: Check code format with black
if: matrix.python-version == 3.9
run: tox -e black
- name: Check package
if: matrix.python-version == 3.9
run: tox -e check_package
- name: Upload to coverage
uses: codecov/codecov-action@v1.2.1
- name: Publish package to PyPI
uses: pypa/gh-action-pypi-publish@master
if: >-
Expand Down
7 changes: 6 additions & 1 deletion setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,16 @@
addopts = -v --junit-xml=test-report.xml
--doctest-modules
--cov=riotctrl --cov-branch
--cov-report=term --cov-report=xml --cov-report=html
--cov-report=term-missing --cov-report=xml
testpaths = riotctrl
markers =
rapidjson

[coverage:report]
exclude_lines = pragma: no cover
raise NotImplementedError
return NotImplemented
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do need to add these lines ? In theory, these kind of things could be tested.

Copy link
Member Author

@miri64 miri64 Sep 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why do need to add these lines ? In theory, these kind of things could be tested.

I was called Mrs "100% percent coverage" by @MrKevinWeiss before, but even I don't test code that states on its own that it is not implemented ;-).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a bit out of scope of this PR anyway. And I think, this only concerns

riotctrl/riotctrl/ctrl.py

Line 237 in ee12bb5

raise NotImplementedError

The "raise NotImplementedError" is useless since it's an abstract method, you could just replace it with .... When instanciating a concrete class that doesn't implement the get_ctrl method, python will raise a TypeError exception telling what method is missing.

Copy link
Contributor

@aabadie aabadie Sep 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

To summarize, I think this whole configuration block should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a bit out of scope of this PR anyway. And I think, this only concerns

riotctrl/riotctrl/ctrl.py

Line 237 in ee12bb5

raise NotImplementedError

But then get a missing coverage there....

Copy link
Contributor

Choose a reason for hiding this comment

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

then get a missing coverage there...

Then just replace this line with ... # pragma: no cover

If you want to ensure a derived class cannot be instanciated without a proper implementation of the get_ctrl method, you can use this test:

def test_not_implemented_factory():
    class MyFactory(riotctrl.ctrl.RIOTCtrlFactoryBase):
        pass

    with pytest.raises(TypeError) as exc_info:
        _ = MyFactory()

    assert exc_info.value.args[0] == (
        "Can't instantiate abstract class MyFactory "
        "with abstract method get_ctrl"
    )

but you definitely don't need to add an exclude_lines config in setup.cfg.

Copy link
Member Author

@miri64 miri64 Sep 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Fine... let's finally get this in and worry about actual coverage later...


[pylint]
reports = no
max-line-length = 88
Expand Down