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

Migrate from (deprecated) nose to pytest #302

Merged
merged 4 commits into from
Dec 14, 2022
Merged

Conversation

daniloegea
Copy link
Collaborator

Pytest and pycoverage require some extra configuration to track test coverage in tests that spawn subprocesses. The new files .coveragerc and sitecustomize.py are intended to enable support for subprocesses.

The test set test_terminal.py is completely skipped when testing on my system and, because of that, coverage test was reporting a low coverage. I still need to see it running in our automation to validate if we still need to skip them. Another workaround would be add a nocover to the Terminal class.

Also:

  • Fix some deprecation warnings in some tests.
  • Remove this task from the TODO list :)

Description

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

Pytest and pycoverage require some extra configuration to track test
coverage in tests that spawn subprocesses. The new files .coveragerc and
sitecustomize.py are intended to enable support for subprocesses.

The test set test_terminal.py is completely skipped when testing on my
system and, because of that, coverage test was reporting a low coverage.
I still need to see it running in our automation to validate if we still
need to skip them. Another workaround would be add a nocover to the
Terminal class.

Also:
  - Fix some deprecation warnings in some tests.
  - Remove this task from the TODO list :)
@daniloegea daniloegea added the RFC Request for comment (don't merge yet) label Nov 30, 2022
@daniloegea daniloegea marked this pull request as draft November 30, 2022 12:23
@slyon slyon self-requested a review December 5, 2022 13:21
@slyon
Copy link
Collaborator

slyon commented Dec 5, 2022

@daniloegea There are some fixes for test_terminal in #289 that you might want to cherry-pick into this PR, so we can close the other one.

@daniloegea daniloegea marked this pull request as ready for review December 5, 2022 14:40
Copy link
Contributor

@sil2100 sil2100 left a comment

Choose a reason for hiding this comment

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

Okay, this looks like good to go. There is a missing newline, but I can fix that when merging. Also left an inline comment.

I checked the output of the CI test run here and compared this with the previous ones - coverage seems fine, tests seem to be ran as they did before. I'll merge this.

@@ -48,4 +48,4 @@ jobs:
dch -v $(git describe --tags) "Autopkgtest CI testing (Jammy)"
- name: Run autopkgtest (incl. build)
run: |
sudo autopkgtest . -U --env=DPKG_GENSYMBOLS_CHECK_LEVEL=0 --env=DEB_BUILD_OPTIONS=nocheck -- lxd autopkgtest/ubuntu/jammy/amd64
sudo autopkgtest . --setup-commands='apt -y install python3-pytest python3-pytest-cov' -U --env=DPKG_GENSYMBOLS_CHECK_LEVEL=0 --env=DEB_BUILD_OPTIONS=nocheck -- lxd autopkgtest/ubuntu/jammy/amd64
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this will no longer be required once we update the netplan packaging (dependencies)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's correct.

TODO Show resolved Hide resolved
@sil2100 sil2100 merged commit 52c572d into canonical:main Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request for comment (don't merge yet)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants