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

Add pip check to workflows #40

Merged
merged 8 commits into from
Nov 4, 2021
Merged

Add pip check to workflows #40

merged 8 commits into from
Nov 4, 2021

Conversation

pvk-developer
Copy link
Member

Resolves #39

@pvk-developer pvk-developer requested review from katxiao, a team and amontanez24 and removed request for katxiao and a team November 2, 2021 14:53
katxiao
katxiao reviewed Nov 2, 2021
View reviewed changes
Copy link
Contributor

@katxiao katxiao left a comment

Choose a reason for hiding this comment

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

Should we add a unit test workflow? These seem like unit tests.

@pvk-developer
Copy link
Member Author

Should we add a unit test workflow? These seem like unit tests.

I think they are more integration than unit, what do you think @csala ? I agree with moving them to integration or unit .

@amontanez24
Copy link
Contributor

Should we add a unit test workflow? These seem like unit tests.

I think they are more integration than unit, what do you think @csala ? I agree with moving them to integration or unit .

I think I see these as unit tests as well, but either way we should add them in before merging this

@csala
Copy link
Contributor

csala commented Nov 4, 2021

Should we add a unit test workflow? These seem like unit tests.

I think they are more integration than unit, what do you think @csala ? I agree with moving them to integration or unit .

I think I see these as unit tests as well, but either way we should add them in before merging this

I agree with @amontanez24 and @katxiao these are unit tests. You moved them to the right place @pvk-developer

csala
csala reviewed Nov 4, 2021
View reviewed changes
Makefile Outdated
.PHONY: test-pytest
test-pytest: ## run all the tests using pytest
invoke pytest
.PHONY: test-integration
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be called test-unit right? Below it is also invoking integration where it should be unit

katxiao
katxiao approved these changes Nov 4, 2021
View reviewed changes
Copy link
Contributor

@katxiao katxiao left a comment

Choose a reason for hiding this comment

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

lgtm!

amontanez24
amontanez24 approved these changes Nov 4, 2021
View reviewed changes
Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

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

One small question but lgtm!

tasks.py
c.run('python -m pytest ./tests/unit --reruns 3')


def coverage(c):
Copy link
Contributor

Choose a reason for hiding this comment

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

why doesn't this one have the @task?

@pvk-developer pvk-developer merged commit fdbfc4b into master Nov 4, 2021
@pvk-developer pvk-developer deleted the issue_39_add_pip_check_to_workflows branch November 4, 2021 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csala csala csala left review comments

@katxiao katxiao katxiao approved these changes

@amontanez24 amontanez24 amontanez24 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add pip check to CI workflows
4 participants
@pvk-developer @amontanez24 @csala @katxiao