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

Add testing workflow and use pytest #315

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

smokestacklightnin
Copy link

@smokestacklightnin smokestacklightnin commented Aug 15, 2024

Here is a summary of what is included in this PR:

  • As part of this workflow, pytest has been added so that we can drop the previous custom test collection system
  • Remove if __name__ == "__main__" section from tests because it is not necessary, or even run, when using pytest. Nontrivial functionality from those sections was preserved.
  • Enable INFO level logging in pytest
  • Add a github workflow that runs tests. A matrix is used to test multiple Python versions
  • Update README.md to reflect changes
  • Add xfail mark to classes with failing tests

@smokestacklightnin smokestacklightnin marked this pull request as ready for review August 16, 2024 04:08
Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

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

Wow, the amount of code we save from doing test collection with pytest is big. Just a few comments about the dependencies, otherwise this looks good.

.github/workflows/ci-test.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

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

Thanks!

@peytondmurray peytondmurray self-requested a review August 20, 2024 23:43
@peytondmurray
Copy link
Contributor

Looks like there are also PicklingErrors - are these related to the problem you mentioned?

Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

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

After discussion, I'm changing this to a request for changes until we understand the PicklingError.

@smokestacklightnin
Copy link
Author

For now, my plan is to add xfail marks to the failing tests to be fixed in the future.

@smokestacklightnin
Copy link
Author

Test workflows are passing now that we xfail and skip classes with failing tests

@smokestacklightnin smokestacklightnin self-assigned this Aug 28, 2024
@smokestacklightnin smokestacklightnin marked this pull request as draft September 11, 2024 22:57
@smokestacklightnin smokestacklightnin changed the title Add testing workflow and use pytest [WIP] Add testing workflow and use pytest Sep 11, 2024
@iindyk
Copy link
Contributor

iindyk commented Sep 12, 2024

this lib is synced from the internal codebase -- the other way does not happen automatically. all the changes will get reverted on the next internal -> github sync. you'd have to make internal change instead

@iindyk
Copy link
Contributor

iindyk commented Sep 13, 2024 via email

@smokestacklightnin smokestacklightnin changed the title [WIP] Add testing workflow and use pytest Add testing workflow and use pytest Sep 15, 2024
@smokestacklightnin smokestacklightnin marked this pull request as ready for review September 15, 2024 04:47
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