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

Maint/tests #204

Merged
merged 8 commits into from
Oct 14, 2024
Merged

Maint/tests #204

merged 8 commits into from
Oct 14, 2024

Conversation

PGijsbers
Copy link
Contributor

@PGijsbers PGijsbers commented Oct 13, 2024

  • Add documentation on using fixtures and mocks in tests.
  • Replace some "integration"-style tests with lighter versions.
  • Add verified user constants

Summary by Sourcery

Refactor tests to improve performance by replacing some integration tests with lighter versions and introduce verified user constants. Add documentation on using fixtures and mocks in tests to guide developers in writing efficient tests.

New Features:

  • Introduce verified user constants for testing purposes, including NO_USER, SOME_USER, OWNER_USER, and ADMIN_USER.

Enhancements:

  • Refactor tests to replace some integration-style tests with lighter versions that use direct function calls and exception assertions instead of API calls.
  • Add a new test file for user-related tests, including parameterized tests for fetching users based on API keys.

Documentation:

  • Add a new documentation section on writing tests, focusing on the use of fixtures and mocks to improve test performance and reliability.

Tests:

  • Add tests for fetching users with different API keys, ensuring correct user data is returned or None for invalid keys.

Copy link

sourcery-ai bot commented Oct 13, 2024

Reviewer's Guide by Sourcery

This pull request focuses on improving the test suite by adding documentation on using fixtures and mocks, replacing some integration-style tests with lighter versions, and adding verified user constants. The changes aim to enhance test performance and provide guidance for writing more efficient tests.

Class diagram for User and UserGroup

classDiagram
    class User {
        int user_id
        Connection _database
        List~UserGroup~ _groups
    }
    class UserGroup {
        <<enumeration>>
        ADMIN
        READ_WRITE
    }
    User --> UserGroup : has
    note for User "Represents a user with associated groups and database connection"
Loading

Class diagram for ApiKey

classDiagram
    class ApiKey {
        <<enumeration>>
        ADMIN
        REGULAR_USER
        OWNER_USER
        INVALID
    }
    note for ApiKey "Represents different types of API keys as string enumerations"
Loading

File-Level Changes

Change Details Files
Added documentation on using fixtures and mocks in tests
  • Created a new file with detailed explanations on test writing best practices
  • Included a comparison of different fixture overheads
  • Provided guidance on when to use mocks vs. database fixtures
  • Added examples of how to implement mocks in tests
docs/contributing/tests.md
mkdocs.yml
Replaced integration-style tests with lighter versions
  • Refactored test_private_dataset_no_user_no_access to use direct function calls instead of API requests
  • Updated test_private_dataset_owner_access to use function calls and database fixtures instead of API requests
  • Removed skipped test cases that were waiting for API key implementation
tests/routers/openml/datasets_test.py
Added verified user constants and refactored user-related code
  • Created constants for different user types (NO_USER, SOME_USER, OWNER_USER, ADMIN_USER)
  • Moved ApiKey enum from conftest.py to users_test.py
  • Added tests to verify user fetching with different API keys
tests/routers/openml/users_test.py
tests/conftest.py
tests/routers/openml/datasets_list_datasets_test.py

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @PGijsbers - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider verifying that all references to ApiKey have been updated after moving it from conftest.py to users_test.py.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟡 Testing: 3 issues found
  • 🟢 Complexity: all looks good
  • 🟡 Documentation: 2 issues found

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

tests/routers/openml/datasets_test.py Outdated Show resolved Hide resolved
user=owner,
user_db=user_test,
expdb_db=expdb_test,
)


@pytest.mark.skip("Not sure how to include apikey in test yet.")
Copy link

Choose a reason for hiding this comment

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

issue (testing): Skipped test remains unaddressed

This test is still skipped. Consider implementing it using the new user constants and connection fixtures, similar to the other updated tests in this file.

tests/routers/openml/users_test.py Outdated Show resolved Hide resolved
docs/contributing/tests.md Outdated Show resolved Hide resolved
docs/contributing/tests.md Show resolved Hide resolved
tests/routers/openml/dataset_tag_test.py Outdated Show resolved Hide resolved
tests/routers/openml/datasets_list_datasets_test.py Outdated Show resolved Hide resolved
tests/routers/openml/datasets_test.py Outdated Show resolved Hide resolved
tests/routers/openml/migration/datasets_migration_test.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@dd9682c). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #204   +/-   ##
=======================================
  Coverage        ?   92.19%           
=======================================
  Files           ?       51           
  Lines           ?     1844           
  Branches        ?      144           
=======================================
  Hits            ?     1700           
  Misses          ?      102           
  Partials        ?       42           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PGijsbers PGijsbers merged commit 23c5df2 into main Oct 14, 2024
5 checks passed
@PGijsbers PGijsbers deleted the maint/tests branch October 14, 2024 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant