Skip to content

Conversation

@adoroszlai
Copy link
Contributor

@adoroszlai adoroszlai commented Jan 6, 2024

What changes were proposed in this pull request?

If flaky-test-check is triggered for a specific method, it checks if the method exists in the test class. This does not work for test methods inherited from parent class.

Test method testValidateBlockLengthWithCommitKey does not exist in TestSecureOzoneRpcClient.java.Stopping!

https://github.com/adoroszlai/ozone/actions/runs/7431424440/job/20222159860#step:3:33

However, the same test method is accepted for the parent class, which cannot be run, since it's abstract.

I propose removing this safety check. If the specified method cannot be run, build output will show Tests run: 0.

https://issues.apache.org/jira/browse/HDDS-10079

How was this patch tested?

https://github.com/adoroszlai/ozone/actions/runs/7431422827

@adoroszlai adoroszlai added the test label Jan 6, 2024
@adoroszlai adoroszlai self-assigned this Jan 6, 2024
@adoroszlai adoroszlai requested a review from sadanand48 January 6, 2024 12:12
@sadanand48
Copy link
Contributor

If the specified method cannot be run, build output will show Tests run: 0.

I guess the job would still pass though giving a false sense of success to user. I'm okay with current change given the parent class problem, but this would mean we need to check thoroughly when someone posts a successful job link. Should we fail if no tests are run?

@adoroszlai
Copy link
Contributor Author

If the specified method cannot be run, build output will show Tests run: 0.

I guess the job would still pass though giving a false sense of success to user. I'm okay with current change given the parent class problem, but this would mean we need to check thoroughly when someone posts a successful job link. Should we fail if no tests are run?

You are right, the job would still pass. I've filed HDDS-10080 to fail fast (after first iteration) in junit.sh if no tests were run.

BTW, this can happen even prior to this patch, e.g. if I try to run TestOzoneRpcClientAbstract.

@adoroszlai
Copy link
Contributor Author

Should we fail if no tests are run?

This is implemented in #6036.

@adoroszlai
Copy link
Contributor Author

@sadanand48 with HDDS-10080 implemented, is this good to go?

Copy link
Contributor

@sadanand48 sadanand48 left a comment

Choose a reason for hiding this comment

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

@sadanand48 with HDDS-10080 implemented, is this good to go?

Yes , Thanks for the fix. LGTM

@adoroszlai adoroszlai merged commit 74fea96 into apache:master Jan 24, 2024
@adoroszlai adoroszlai deleted the HDDS-10079 branch January 24, 2024 17:22
@adoroszlai
Copy link
Contributor Author

Thanks @sadanand48 for the review.

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.

2 participants