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

Define db and ssh test markers and run db tests in ci pipeline #279

Merged
merged 2 commits into from
Sep 3, 2020

Conversation

jenhagg
Copy link
Collaborator

@jenhagg jenhagg commented Aug 31, 2020

Purpose

Allow selecting db tests specifically, or filtering them out to run other integration tests, e.g. pytest -m 'not db'. Run db tests in github action.

What it does

Define the db mark in pytest.ini and apply it to relevant tests, in addition to the integration mark. Also defined the ssh mark to deselect those from github test runs, while selecting just those locally. New step was added to the ci .yml to start the db container, enabling tests to run against it.

Time to review

10 min

@jenhagg jenhagg requested a review from danielolsen August 31, 2020 22:50
@danielolsen
Copy link
Contributor

danielolsen commented Aug 31, 2020

If we are marking these tests as db, should we remove the integration mark? They need to connect to the local db, not the server.

@rouille
Copy link
Collaborator

rouille commented Sep 1, 2020

If we are marking these tests as db, should we remove the integration mark? They need to connect to the local db, not the server.

I agree with @danielolsen. Also, perhaps the marker name integration is not well chosen or could be more explicit. What do you think of ssh, server or ssh connection?

@jenhagg
Copy link
Collaborator Author

jenhagg commented Sep 1, 2020

I suppose we could name the marks anything as long as we can select subsets of tests for our purposes. My thinking was that integration means tests that have any external dependency - server connection, db, 3rd party api, etc. So I think that's a useful thing to keep, but adding the db mark lets us select those separately, and we can still run the combinations we currently want. Open to other ideas though.

@danielolsen
Copy link
Contributor

I suppose we could name the marks anything as long as we can select subsets of tests for our purposes. My thinking was that integration means tests that have any external dependency - server connection, db, 3rd party api, etc. So I think that's a useful thing to keep, but adding the db mark lets us select those separately, and we can still run the combinations we currently want. Open to other ideas though.

I definitely see the value in having a 'no external dependencies' flag to run with pytest. Right now the description for integration talks about sshing to the server, so maybe we want three tags: integration, db, and ssh, where ssh is a subset of integration, and perhaps db is too (can we run the db tests on github?).

@jenhagg
Copy link
Collaborator Author

jenhagg commented Sep 1, 2020

I will update the description of integration. We should be able to run db on github soon, so in that case we might want to select not ssh, but I think until then the ssh mark would be redundant?

edit: I actually have a branch that was able to run the db tests in github, so I can add the ssh mark and include that in this PR
edit: I integrated those changes, think it's good now

@jenhagg jenhagg self-assigned this Sep 2, 2020
@jenhagg jenhagg added this to the Movin' Out milestone Sep 2, 2020
- name: Start postgres container
run: |
docker-compose -f stack.yml up -d
while ! nc -z localhost 5432; do sleep 1; done;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The loop waits for the container to start, otherwise tests can fail due to race condition. nc -z checks if the port is listening.

@jenhagg jenhagg changed the title test: add separate test marker for db tests Define db and ssh test markers and run db tests in ci pipeline Sep 2, 2020
Comment on lines -9 to -12
strategy:
matrix:
python-version: [3.6, 3.7, 3.8]

Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on with these changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They kinda snuck in from the branch I was testing on, but kept it since the simpler approach seemed better. Github treats 3.x as the latest minor release which I think is currently 3.8.5, so should be compatible with 3.8.3 which we mainly use. I think the main reason to test older versions is if we were distributing this for installation in other environments and wanted the flexibility. So I didn't see it adding much value, but don't mind keeping it if there's a use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

Copy link
Contributor

@danielolsen danielolsen left a comment

Choose a reason for hiding this comment

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

Thanks! This will definitely make local development easier.

@jenhagg jenhagg merged commit 639cfb5 into develop Sep 3, 2020
@jenhagg jenhagg deleted the jon/marker branch September 3, 2020 19:07
@ahurli ahurli mentioned this pull request Mar 11, 2021
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