Skip to content

feat: add tests versioning - #93

Open
kgwizdz-splunk wants to merge 4 commits into
mainfrom
feat/add-tests-versioning
Open

feat: add tests versioning#93
kgwizdz-splunk wants to merge 4 commits into
mainfrom
feat/add-tests-versioning

Conversation

@kgwizdz-splunk

Copy link
Copy Markdown

No description provided.

@srv-rr-github-token

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

Copy link
Copy Markdown
Collaborator

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

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

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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