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

Jump to bottom

Migrate to pytest #584

Merged
merged 11 commits into from
Jan 31, 2022
Merged

Migrate to pytest #584

merged 11 commits into from
Jan 31, 2022

Conversation

arturponinski
Copy link
Collaborator

This PR has the objective of migrating the current nosetest runner to pytest.
Successfully ran the whole test suite against py3.8 with the exception of bcolz.
This the minimal effort and test suite code change required to use pytest.

Changes

  1. The existing conditional checks have been rewritten / added to conform with pytest assertions
  2. Expected exceptions rewritten to use .raises
  3. Tests to be skipped are skipped using pytest
  4. Additionally a fix to close the opened connections in _iter_sqlalchemy_engine has been commited

Checklist

Checklist for for pull requests including new code and/or changes to existing code...

  • Code
    • Includes unit tests
    • New functions have docstrings with examples that can be run with doctest
    • New functions are included in API docs
    • Docstrings include notes for any changes to API or behaviour
    • All changes documented in docs/changes.rst
  • Testing
    • (Optional) Tested local against remote servers
    • Travis CI passes (unit tests run under Linux)
    • AppVeyor CI passes (unit tests run under Windows)
    • Unit test coverage has not decreased (see Coveralls)
  • Changes
    • (Optional) Just a proof of concept
    • (Optional) Work in progress
    • Ready to review
    • Ready to merge

@arturponinski arturponinski mentioned this pull request Jan 28, 2022
Closed
@coveralls
Copy link

coveralls commented Jan 28, 2022 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 90.871% when pulling 388ee4a on arturponinski:migrate-to-pytest into c29120e on petl-developers:master.

@arturponinski
Copy link
Collaborator Author

@juarezr could you give me a hand regarding failing Codacy Security Scan ?

juarezr
juarezr approved these changes Jan 28, 2022
View reviewed changes
Copy link
Member

@juarezr juarezr left a comment

Choose a reason for hiding this comment

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

requirements-tests.txt Outdated
@@ -1,6 +1,8 @@
wheel
setuptools
nose
Copy link
Member

Choose a reason for hiding this comment

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

We need nose yet after this migration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Definitely not 9ff3435

tox.ini Outdated
@@ -18,7 +18,7 @@ setenv =
py36,py37,py38,py39: PY_MAJOR_VERSION = py3
commands =
py27,py36,py38,py39: pytest --cov=petl petl
py37: nosetests -v --with-coverage --cover-package=petl --with-doctest --doctest-options=+NORMALIZE_WHITESPACE petl -I"csv_py2\.py" -I"db\.py"
py37: pytest --cov=petl petl
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need to split py37 anymore because you solved/removed the previous hack here. Right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right 9ff3435

.github/workflows/test-changes.yml
@@ -122,7 +122,7 @@ jobs:
python -m pip install -r requirements-formats.txt
echo "::endgroup::"
echo "::group::Perform doc test execution with coverage"
nosetests -v --with-coverage --cover-package=petl --with-doctest --doctest-options=+NORMALIZE_WHITESPACE petl -I"csv_py2\.py" -I"db\.py"
pytest --cov=petl petl
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering what would be the best approach here:

  • Calling pytest directly from here as I did with nose, or...
  • Calling pytest through tox as one would in command line

Not a requirement, off course. Just getting any possible opinion on the subject.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now, I would leave it as is, calling pytest directly. Ideally, however I would prefer to align the call as one would do it locally. I recall that in one of my previous PRs there were some slight discrepancies between calling tox locally and how one of the functions behaved on CI.
I would even consider a Docker approach, or docker-compose with db and remote services (instead of calling docker run)

@juarezr juarezr added Build CI CD Issues affecting github pypi readthedocs coveralls conda-forge Maintainability Issues for code modernization, improve development, testing Usability Improvement that benefit petl usage for end users labels Jan 28, 2022
@juarezr juarezr linked an issue Jan 28, 2022 that may be closed by this pull request
Migrate to pytest #453
Closed
@juarezr
Copy link
Member

juarezr commented Jan 29, 2022

@juarezr could you give me a hand regarding failing Codacy Security Scan ?

Missed your message.

The failure is not related with your changes. As your code does not change runtime code, it's safe to assume that we are not spreading any security issue in the released package.

Probably the check did break because of some random reason. It should be running even independently of any other changes.

@arturponinski
Copy link
Collaborator Author

arturponinski commented Jan 30, 2022 •
edited
Loading

Glad to hear that, and some nice catches from you!
I think it is good to go now.
After the merge, work on reducing code duplicates, using pytest goodies like fixtures can slowly begin - I could start with splitting test_db_server, it can be nicely split per library with some common parts shared among test clasess.

@juarezr juarezr mentioned this pull request Jan 31, 2022
Closed
@juarezr
Copy link
Member

juarezr commented Jan 31, 2022

@arturponinski,

Nice job!

@juarezr juarezr merged commit 9f12eaf into petl-developers:master Jan 31, 2022
This was referenced Jan 31, 2022
Merged
Merged
Merged
Closed
@juarezr juarezr added this to the v2.0 milestone Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juarezr juarezr juarezr approved these changes

Assignees
No one assigned
Labels
Build CI CD Issues affecting github pypi readthedocs coveralls conda-forge Maintainability Issues for code modernization, improve development, testing Usability Improvement that benefit petl usage for end users
Projects
None yet
Milestone
v2.0
Development

Successfully merging this pull request may close these issues.

Migrate to pytest
3 participants
@arturponinski @coveralls @juarezr