Skip to content

Conversation

nowinkeyy
Copy link
Contributor

@nowinkeyy nowinkeyy commented Sep 22, 2022

Make sure set the target branch to develop

What is the purpose of the change

fix #5122

Brief changelog

XX

Verifying this change

XXXX

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

  • Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
  • Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@nowinkeyy nowinkeyy changed the title Enable checkstyle for test code [ISSUE #5122] Enable checkstyle for test code Sep 23, 2022
@aaron-ai
Copy link
Member

Could you fix the test of DefaultMQAdminExtTest#fetchAllTopicList to pass the CI pipeline? @nowinkeyy

image

@nowinkeyy
Copy link
Contributor Author

Could you fix the test of DefaultMQAdminExtTest#fetchAllTopicList to pass the CI pipeline? @nowinkeyy

image

After i git reset the branch, i also saw other test errors. I try to fix it all.

@lizhanhui
Copy link
Contributor

+1 to enable style check for test assets.

@nowinkeyy
Copy link
Contributor Author

+1 to enable style check for test assets.

Bro are you gonna fix the test code to. Seems like I found a lot of test code that didn't work? (i'm not sure)

@nowinkeyy
Copy link
Contributor Author

Could you fix the test of DefaultMQAdminExtTest#fetchAllTopicList to pass the CI pipeline? @nowinkeyy

image
image

After i change 2 to 1, an error occurs. It seems 2 is correct?

@aaron-ai
Copy link
Member

aaron-ai commented Sep 26, 2022

@nowinkeyy This is a huge pull request. If you are not sure about if your modifications affected the CI result which is required to pass, it may be a good idea to split it by the module. I am pleased to see this feature could be merge as soon as possible.

@nowinkeyy
Copy link
Contributor Author

@nowinkeyy This is a huge pull request. If you are not sure about if your modifications affected the CI result which is required to pass, it maybe a good idea to split it by the module. I am pleased to see this feature could be merge as soon as possible.

Do you mean to submit PR according to module? If so, i will do so.

@lizhanhui
Copy link
Contributor

Do you mean to submit PR according to module? If so, i will do so.

If the changes are too large, it is OK and advised to split into multiple pull requests. This way, we may have quality reviews and quick merge for those that are simple, and obvious enough.

@nowinkeyy
Copy link
Contributor Author

Do you mean to submit PR according to module? If so, i will do so.

If the changes are too large, it is OK and advised to split into multiple pull requests. This way, we may have quality reviews and quick merge for those that are simple, and obvious enough.

I got it!

@nowinkeyy
Copy link
Contributor Author

Hi @aaron-ai , My new PR #5193.

@tsunghanjacktsai
Copy link
Contributor

Maybe close this PR and refer it to the issue itself?

@nowinkeyy nowinkeyy closed this Oct 8, 2022
@nowinkeyy nowinkeyy deleted the rocketmq-5122 branch October 14, 2022 06:37
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.

Enable checkstyle for test code
4 participants