Skip to content

Issue #4105: include s3_management tests in unit test discovery#4642

Open
rwilliamspbg-ops wants to merge 6 commits into
ROCm:mainfrom
rwilliamspbg-ops:users/ryan/issue-4105-unit-test-scope
Open

Issue #4105: include s3_management tests in unit test discovery#4642
rwilliamspbg-ops wants to merge 6 commits into
ROCm:mainfrom
rwilliamspbg-ops:users/ryan/issue-4105-unit-test-scope

Conversation

@rwilliamspbg-ops
Copy link
Copy Markdown

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

@ScottTodd
Copy link
Copy Markdown
Member

Duplicate of #4107 ?

@rwilliamspbg-ops
Copy link
Copy Markdown
Author

Motivation

Issue #4105 reported that build_tools/third_party/s3_management/test_update_dependencies.py was not discovered by the build_tools unit test workflow.

Technical Details

  • Added third_party/s3_management to build_tools/pyproject.toml test discovery.
  • Renamed the test file to update_dependencies_test.py so it matches the repository's *_test.py convention.
  • Kept the test close to the third-party code it validates.
  • Updated one expectation to match the current policy in update_dependencies.py (cp314 is allowed).

Test Plan

  • cd build_tools
  • python -m pytest -vv third_party/s3_management/update_dependencies_test.py

Test Result

  • 37 passed

Submission Checklist

Copy link
Copy Markdown
Contributor

@HereThereBeDragons HereThereBeDragons left a comment

Choose a reason for hiding this comment

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

thank you for your contribution

could you please update pr title and description and describing why and what is done in the pr? as they will become the commit message. i guess you could just copy your other message into the description.

otherwise looks good for me if the tests are passing

Copy link
Copy Markdown
Contributor

@HereThereBeDragons HereThereBeDragons left a comment

Choose a reason for hiding this comment

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

looking at the tests:
could you please create a new tests/ folder under s3_management/ and move the file there?
and it then needs to be excluded for the coverage.
you can get inspired by #4107

if you dont want to do those changes (as it more or less what #4107 already has), i would close this pr in favor of #4107

please let me know how you want to continue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: TODO

Development

Successfully merging this pull request may close these issues.

4 participants