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

minimall add pytest #1859

Merged
merged 4 commits into from
Apr 16, 2023
Merged

minimall add pytest #1859

merged 4 commits into from
Apr 16, 2023

Conversation

Swiftyos
Copy link
Contributor

Background

This PR aims to introduce pytest as the testing framework for our project. While the project currently uses unittest, pytest offers several advantages such as improved test discovery, more powerful assertions, and a rich ecosystem of plugins. This change will help improve the quality and maintainability of our test suite.

As part of this PR, we are also adding pytest as a pre-commit check to ensure that tests are passing before any code is committed to the repository. Additionally, we have included the pytest-integration plugin, which allows us to better manage and separate integration tests from unit tests.

Changes

  • Added pytest and pytest-integration to the project's dependencies.
  • Updated the test runner configuration to use pytest.
  • Configured pytest as a pre-commit check.

Documentation

The changes in this PR are documented in the following ways:

  • Updated the README.md file to mention the use of pytest for testing.
  • Added comments in the code where necessary to explain the usage of pytest features.
  • Updated the project's documentation to mention the use of pytest and the pytest-integration plugin.

Test Plan

To ensure the functionality of this PR, the following tests were performed:

  • Ran the existing test suite using pytest to confirm that all tests pass.
  • Created new tests using the pytest syntax and confirmed that they pass.
  • Checked that the pre-commit check correctly runs pytest and prevents committing code with failing tests.
  • Ran integration tests using the pytest-integration plugin to confirm that they are correctly separated from unit tests.

PR Quality Checklist

  • My pull request is atomic and focuses on a single change.
  • I have thoroughly tested my changes with multiple different prompts.
  • I have considered potential risks and mitigations for my changes.
  • I have documented my changes clearly and comprehensively.
  • I have not snuck in any "extra" small tweaks changes

nponeccop
nponeccop previously approved these changes Apr 16, 2023
@nponeccop nponeccop added the B7 label Apr 16, 2023
@drikusroor
Copy link
Contributor

@0xArty Changing to pytest seems like a good idea to me. I have some questions and comments about the process of migrating to pytest, especially since I have been re-organizing some tests as well:

  1. I don't see all tests migrated to pytest in this PR. Is that even needed or can pytest also run the already present unittest style of tests? And if not, are you planning on migrating those to pytest and/or do you want help with that?
  2. I noticed you haven't updated ci.yml for running automated tests using pytest now. Is that something we could do in this PR as well? Either as a replacement for the unittest job or alongside it.
  3. I saw you used the @pytest.mark.integration_test annotation. Is that the common way of letting pytest know what are unit tests and what are integration tests? As some tests are located in the tests directory, and some in the tests/unit and tests/integration directories, how would you feel about moving integration tests to the tests/integration directory and unit tests to the tests/unit directory?

Let me know what you think, I'm open for debate. We could also try & conjure up some ADRs together and split the work.

@Swiftyos
Copy link
Contributor Author

Hey @drikusroor Thanks so much for your input and suggestions on the pytest migration. Here's some insight into the current plan:

  1. You're right, pytest can actually pick up and run unittests. The intention is to eventually migrate everything to pytest, but for now, I'm focusing on keeping this PR minimal and atomic to maintain a manageable scope.
  2. I agree that switching to pytest is a good move, but as mentioned earlier, I'm trying to keep the changes in this PR to a minimum. We can definitely consider updating ci.yml for pytest in a future PR.
  3. Yes, using the @pytest.mark.integration_test annotation is a common way to distinguish between unit tests and integration tests with the pytest-integration library. I know the folder structure for our tests is a bit disorganized right now, and I agree that we should work towards a more standardized approach. But let's take it one step at a time.

I truly appreciate your input and willingness to collaborate. Your idea of dividing the work is great, and I'd love to help. Let's work together to make this transition as smooth as possible!

This PR is just doing the minimum to get pytest supported.

@nponeccop
Copy link
Contributor

@0xArty There are conflicts now

@p-i- p-i- merged commit 627533b into Significant-Gravitas:master Apr 16, 2023
@nponeccop nponeccop mentioned this pull request Apr 17, 2023
1 task
laixiao pushed a commit to laixiao/Auto-GPT-code that referenced this pull request Apr 18, 2023
* minimall add pytest

* updated docs and pytest command

* proveted milvus integration test running if milvus is not installed
sindlinger pushed a commit to Orgsindlinger/Auto-GPT-WebUI that referenced this pull request Sep 25, 2024
* minimall add pytest

* updated docs and pytest command

* proveted milvus integration test running if milvus is not installed
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.

4 participants