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

[INF] Set multi-test env to compat dependency and system #1143

Open
wants to merge 48 commits into
base: dev
Choose a base branch
from

Conversation

Zeroto521
Copy link
Member

@Zeroto521 Zeroto521 commented Aug 11, 2022

PR Description

Please describe the changes proposed in the pull request:

Aim:

  • Set multi-test environment
  • Compat different dependencies and systems

ToDo/Doing:

  • Set latest env: test pyjanitor work with the latest dependencies to get the minimal python version
  • Set minimal env: get the minimal version of dependencies

Part of #1133

PR Checklist

Please ensure that you have done the following:

  1. PR in from a fork off your branch. Do not PR from <your_username>:dev, but rather from <your_username>:<feature-branch_name>.
  1. If you're not on the contributors list, add yourself to AUTHORS.md.
  1. Add a line to CHANGELOG.md under the latest version header (i.e. the one that is "on deck") describing the contribution.
    • Do use some discretion here; if there are multiple PRs that are related, keep them in a single line.

Automatic checks

There will be automatic checks run on the PR. These include:

  • Building a preview of the docs on Netlify
  • Automatically linting the code
  • Making sure the code is documented
  • Making sure that all tests are passed
  • Making sure that code coverage doesn't go down.

Relevant Reviewers

Please tag maintainers to review.

@Zeroto521
Copy link
Member Author

Why does the test action don't work?
Is it a project setting problem?

@ericmjl
Copy link
Member

ericmjl commented Aug 11, 2022

I think it's because the PR is a draft, maybe?

@Zeroto521 Zeroto521 marked this pull request as ready for review August 11, 2022 13:08
@Zeroto521
Copy link
Member Author

Zeroto521 commented Aug 11, 2022

I opened this PR and pushed an empty commit, but it still can't trigger test ci.

@Zeroto521 Zeroto521 changed the title INF: Set multi-test env to compat dependency and system [INF] Set multi-test env to compat dependency and system Aug 11, 2022
@Zeroto521
Copy link
Member Author

Zeroto521 commented Aug 12, 2022

It seems these functions really have problems.

Need to fix:

I will have a look at test_encode_categorical.py and test__select_column.py

@Zeroto521
Copy link
Member Author

Zeroto521 commented Aug 20, 2022

I just skip tests/test_documentation_build.py.
I thought we should test documentation in documentation building CI not in the test CI.
I want to keep a clean environment, don't want to mix them (test dependencies and documentation dependencies) up.

EDIT: done #1183

@Zeroto521
Copy link
Member Author

Zeroto521 commented Oct 30, 2022

This goal will be done at the next PR.

Set minimal env: get the minimal version of dependencies

It's ready to be reviewed.

@Zeroto521
Copy link
Member Author

Zeroto521 commented Nov 14, 2022

Why these py3.8 envs didn't get error?
#1200

@Zeroto521 Zeroto521 mentioned this pull request Nov 19, 2022
3 tasks
@Zeroto521 Zeroto521 requested a review from ericmjl November 26, 2022 13:13
@Zeroto521
Copy link
Member Author

Why these py3.8 envs didn't get error? #1200

Is #889 the reason?

@ericmjl
Copy link
Member

ericmjl commented Dec 6, 2022

@Zeroto521 I'm not sure why, but I think testing on py3.8 might become less relevant once py3.12 is released.

Copy link
Member

@ericmjl ericmjl left a comment

Choose a reason for hiding this comment

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

@Zeroto521 only one set of change requests from me, the other being a question for you to address (but not necessarily act on in this PR).

Thank you for helping out here!

- "3.8"
- "3.9"
- "3.10"
env: [ci/envs/latest.yaml]
Copy link
Member

Choose a reason for hiding this comment

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

@Zeroto521 would you recommend also testing on the development environment as defined in environment-dev.yml?

run: pytest -v -r a -n auto --color=yes --durations=0 --cov=janitor --cov-append --cov-report term-missing --cov-report xml tests -m "not turtle"

- name: Run turtle unit tests
run: pytest -v -r a -n auto --color=yes --durations=0 --cov=janitor --cov-append --cov-report term-missing --cov-report xml tests -m "turtle"
Copy link
Member

Choose a reason for hiding this comment

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

Historically we've been able to reduce testing time by running the turtle and not turtle tests in parallel. Codecov will also get updated correctly as long as --cov-append is called, if I remember correctly. Can we reinstate running turtle and not turtle tests here, please?

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.

3 participants