Skip to content

feat: add tests versioning #93

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kgwizdz-splunk
Copy link

No description provided.

@srv-rr-github-token
Copy link
Collaborator

srv-rr-github-token commented Apr 17, 2025

Let me review this pull request.

Suggested pull request title: feat: add version-based test filtering support

Thank you for implementing this valuable feature for version-specific test filtering. The implementation is clean and well-documented, with good consideration for backward compatibility by making the --ta-version parameter optional.

The changes could benefit from a few minor improvements to make the feature more robust and user-friendly.

Here are my specific suggestions:

  1. In hooks.py, consider adding validation for the --ta-version argument format when it's provided. While the Version class from packaging.version will handle this, providing a more user-friendly error message early in the process would be helpful.

  2. In utils.py, the _collect_out_of_range_tests_by_version function uses a very high default max version ("9999.9999.9999"). Consider using float('inf') or implementing a more elegant solution for "no upper bound" scenarios.

  3. In the documentation (framework_deepdive.md), it would be helpful to add examples of version specifications that are considered valid, and mention that the versioning follows PEP 440.

  4. Consider adding a test case that demonstrates the version filtering functionality to ensure it works as expected.

After implementing these suggestions, the code will be ready for merging.

This comment was added by our PR Review Assistant Bot. Please kindly acknowledge that
while we're doing our best to keep these comments up to very high standards, they may occasionally be
incorrect. Suggestions offered by the Bot are only intended as points for consideration and no statements
by this bot alone can be considered grounds for merging of any pull request. Remember to seek a review
from a human co-worker.

To reply to the review and engage Review Bot in further conversation, start your comment with the words Review bot:

@srv-rr-github-token
Copy link
Collaborator

srv-rr-github-token commented Apr 17, 2025

Suggested pull request title: feat: add test version filtering with version_range decorator

Thank you for implementing the test versioning feature. The code is well-organized and includes both implementation and documentation, which is great to see. The version range decorator will be very useful for managing version-specific test cases.

There are a few areas where the implementation could be improved for better robustness and clarity.

Here are the specific suggestions:

  1. In pytest_plugin/utils.py, the version comparison logic should handle invalid version strings gracefully:

    • Add try-catch block around Version(version_str) to handle invalid version format
    • Consider adding validation for min/max version strings in the decorator
    • Add logging for cases when version parsing fails
  2. In pytest_plugin/hooks.py, the version range marker description could be more detailed:

    config.addinivalue_line(
        "markers",
        "version_range(min=None, max=None): mark test with applicable version range. Both min and max are inclusive."
    )
  3. Documentation in framework_deepdive.md could use a few improvements:

    • Add examples of how to specify version ranges with only min or only max version
    • Include information about version string format requirements
    • Mention that ranges are inclusive on both ends
  4. Consider adding tests for the version range functionality itself to ensure it works correctly with various version combinations.

After implementing these suggestions, the code will be ready for merging. The core functionality is solid, and these changes will just make it more robust and user-friendly.

This comment was added by our PR Review Assistant Bot. Please kindly acknowledge that
while we're doing our best to keep these comments up to very high standards, they may occasionally be
incorrect. Suggestions offered by the Bot are only intended as points for consideration and no statements
by this bot alone can be considered grounds for merging of any pull request. Remember to seek a review
from a human co-worker.

To reply to the review and engage Review Bot in further conversation, start your comment with the words Review bot:

)
config.hook.pytest_deselected(items=excluded_items)
items[:] = [item for item in items if item not in excluded_items]

skipped_tests = _collect_skipped_tests(items)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Framework need to know what test has been skipped to update internal data accordingly. Please update item removal code to save skipped test list in skipped_tests variable

Copy link
Collaborator

Choose a reason for hiding this comment

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

One more thing - PytestConfigAdapter is not updated to handle ta_version property

@hsekowski-splunk
Copy link
Collaborator

@kgwizdz-splunk ,
Let's go through https://github.com/splunk/addonfactory-ucc-test/tree/develop to main
There are other things we are working on that potentially may come to main with your modifications

@artemrys
Copy link
Member

Would appreciate some tests as well.

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.

5 participants