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

chore: initialize pytest and create workflow #155

Closed
wants to merge 13 commits into from

Conversation

BallardRobinett
Copy link
Contributor

@BallardRobinett BallardRobinett commented Feb 13, 2024

Issue: https://github.com/orgs/bcgov/projects/123/views/2?pane=issue&itemId=49861478

Initializing pytest with a sample test, creating the workflow, and updating the readme with instructions.

To test manually:

In the terminal navigate to the /server/ directory
Do the command pytest
You should get a message like "[100%] 1 passed in 0.00s"

To test the workflow, it should appear as one of the tests that is automatically run when this PR is created.

@BallardRobinett BallardRobinett self-assigned this Feb 13, 2024
Copy link
Contributor

@pbastia pbastia left a comment

Choose a reason for hiding this comment

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

Nice work! We just need to make sure we run pytest in the proper virtual environment, with our poetry setup

Comment on lines 5 to 8
branches: [main, develop, 73-initialize-pytest]
pull_request:
branches: [main, develop, 73-initialize-pytest]

Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove your branch now

Suggested change
branches: [main, develop, 73-initialize-pytest]
pull_request:
branches: [main, develop, 73-initialize-pytest]
branches: [main, develop]
pull_request:
branches: [main, develop]

Since we have the pull_request here, you don't need to explicitely have your branch name in here, you can just open a Draft PR in github and it'll run the actions for you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, I keep forgetting this. Thanks Pierre!

requirements.txt Outdated
@@ -1,2 +1,3 @@
pre-commit==2.19.0
gitlint==0.19.1
pytest==8.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think pytest should be added as a dependency for the whole app (we use this top-level requirements file for pre-commit).
pytest is already installed as a dev dependency in /server/pyproject.toml

Comment on lines 22 to 28
- name: Install dependencies
run: |
python -m pip install --upgrade pip
if [ -f requirements.txt ]; then pip install -r requirements.txt; fi
- name: Test with pytest
run: |
pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

This will install the top-level python dependencies, which are separate from our poetry venv and Django app.

You can look at the registration action that runs poetry pytest in the /server/ folder:

https://github.com/bcgov/cas-registration/blob/16db8cf02c7cf429c2da23fe7772c227d595b05b/.github/workflows/test.yaml#L225

uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
- name: Install dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

This job could greately benefit from caching to avoid reinstalling all dependencies every run

@rdromey rdromey closed this Apr 23, 2024
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