Skip to content

Revert "Use TEST_TYPE directly as categories for test filtering"#4135

Open
aalabsi-amd wants to merge 1 commit into
mainfrom
revert-4008-users/dravindr/tf_test_types
Open

Revert "Use TEST_TYPE directly as categories for test filtering"#4135
aalabsi-amd wants to merge 1 commit into
mainfrom
revert-4008-users/dravindr/tf_test_types

Conversation

@aalabsi-amd
Copy link
Copy Markdown
Contributor

Reverts #4008

This caused issues in miopen CI running the unintended full category instead of the standard.
CI is blocking PRs now, need to unblock until we address this change from miopen.

Comment on lines -225 to -231
if category not in VALID_TEST_CATEGORIES:
print(
f"ERROR: Invalid TEST_TYPE '{TEST_TYPE}'. "
f"Must be one of: {', '.join(sorted(VALID_TEST_CATEGORIES))}. "
f"Falling back to 'quick'.",
file=sys.stderr,
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I also came across this code yesterday. This should not have been merged as-is. This should have raised an exception instead of silently falling back to "quick" (stderr does not count if no one is checking it)

See the style guide: https://github.com/ROCm/TheRock/blob/main/docs/development/style_guides/python_style_guide.md#fail-fast-behavior

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(then we could also have a unit test checking that only valid test categories are used, so we don't need to wait until late in the CI pipeline to see issues)

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.

3 participants