proposal: Adding integration tests when submitting a PR

[seeflood]

What problem does this proposal solve

Our code review process is slow, and hard. I want to make PR review faster. We added some linters in CI, but that's far not enough. PR review is still hard 😢 and still slow

Analyze

Model

We can model "PR review" into different phases:

• Triage

• Design review Review the design docs (if any) or API definition.

• Test case review Review there are enough test cases covering different corner cases.

• Test

  • pass existing test cases
  • add new UT
  • add new integration test

• Code review Finally, we start to review code details.

Pain points

• Some PR has no detailed design documentation. A good design doc makes PR review much much easier, e.g. https://github.com/dapr/dapr/issues/2988
• Some PR don't have integration tests, which means it might not pass self-tests. It will take reviewers much time to review when a PR didn't do a self-test. They might have UT, but as we all know, UT can't find out bugs :)

Solutions

We add new PR specification. PR with new features should:

1. add new integration test cases We can take the "quickstart" markdown docs as "integration tests" For example, to review and test #669 , I submitted https://github.com/mosn/layotto/pull/698 as the integration tests

2. add those tests into CI. After writing the quickstart docs, a PR should add it into the CI.

For more details, see https://mosn.io/layotto/#/zh/development/test-quickstart?id=step-5-%e4%bf%ae%e6%94%b9-ci%e8%87%aa%e5%8a%a8%e6%b5%8b%e8%af%95%e6%96%b0%e5%86%99%e7%9a%84-quickstart-%e6%96%87%e6%a1%a3

3. After new tests added into CI and all tests passed (which means this PR has been fully self-tested ), reviewers start to review

[seeflood]

chinese: 鉴于现在社区 pr view 比较慢，且比较难，我想到个提案，要求以后提 PR 时必须写新的集成测试、把新的集成测试加进CI 里，测试通过了才代表“本 PR 已经充分自测过”，Reviewer 开始 review 在此之前，reviewer 只需要关注 “设计是否合理” 和 "集成测试用例全不全"。通过 集成测试后再 review 代码细节

可以帮忙一起 review 下提案哈 @mosn/layotto-commiter @mosn/layotto-pmc @mosn/layotto-admins @mosn/layotto-member

[nobodyiam]

There are many different types of pull requests, e.g. bug fix, documentation, demo, new feature, etc. I suppose only those new feature pull requests are concerned here?

[wenxuwan]

may be ut is better, ci complexity is too high。Can ask users to post their own test reports。

[seeflood]

There are many different types of pull requests, e.g. bug fix, documentation, demo, new feature, etc. I suppose only those new feature pull requests are concerned here?

Yes, only those new feature pull requests are concerned here.

[seeflood]

may be ut is better

UT can't find out bugs. But integration tests can. Examples are #669 and https://github.com/mosn/layotto/pull/574

ci complexity is too high。

You can list the pain points you met. We can try to improve it. For me it's not complex, you just need to add your markdown file into the test-quickstart.sh, see https://mosn.io/layotto/#/zh/development/test-quickstart?id=step-5-%e4%bf%ae%e6%94%b9-ci%e8%87%aa%e5%8a%a8%e6%b5%8b%e8%af%95%e6%96%b0%e5%86%99%e7%9a%84-quickstart-%e6%96%87%e6%a1%a3

Can ask users to post their own test reports。

Manual testing has the following disadvantages:

• can't help reviewer review the PR. When there are no enough doc, reviewers need to run demos to understand the code. Otherwise it's too hard to review without docs and demos.
• Manual testing cannot help future regression testing. Automated tests helps a lot in the future

[Xunzhuo]

Our code review process is slow, and hard. I want to make PR review faster.

From my perspective, the current issue help maintainers review PR faster is not all related to the CI and it is only related to the PRs that are very large.

I think it is an answer to How to make big refactoring tasks or feature tasks?, we can add more integration tests and I agree, but in a large PR, it can not find all bugs too.

So the solution is that "separate the large PRs into small ones", it is a good way to resolve the help maintainers review PR faster issue in almost every projects.

And it is a suggestion to all contributors, if a feature/refactoring task is too big, consider creating an issue, and separating the tasks into an issue and add some descriptions on it, if the maintainers in community think it is good to go on, the tasks can get started to go on, if not, separate the tasks properly again according to the suggestions of maintainers. Take this one as an example https://github.com/mosn/layotto/issues/532, imagine that if I start only one PR to fix all tasks in this issue, it could drive reviewers crazy.

Maintainers should control the scale of a PR, if it is too big, it is hard to maintain, this is the only way to solve this issue.

Contributors and maintainers should keep a good communications by actively exchanging messages and ideas, it can prevent this issue deeply, not just rely on the CI, and make it too heavy by adding countless complex integration tests.

@mosn/layotto-commiter @mosn/layotto-pmc @mosn/layotto-admins @mosn/layotto-member How do you forks think?

[wenxuwan]

may be ut is better

UT can't find out bugs. But integration tests can. Examples are #669 and #574

ci complexity is too high。

You can list the pain points you met. We can try to improve it. For me it's not complex, you just need to add your markdown file into the test-quickstart.sh, see https://mosn.io/layotto/#/zh/development/test-quickstart?id=step-5-%e4%bf%ae%e6%94%b9-ci%e8%87%aa%e5%8a%a8%e6%b5%8b%e8%af%95%e6%96%b0%e5%86%99%e7%9a%84-quickstart-%e6%96%87%e6%a1%a3

Can ask users to post their own test reports。

Manual testing has the following disadvantages:

• can't help reviewer review the PR. When there are no enough doc, reviewers need to run demos to understand the code. Otherwise it's too hard to review without docs and demos.
• Manual testing cannot help future regression testing. Automated tests helps a lot in the future

If i want to run oss ci , i need provide my ak、sk in configure file？

[wenxuwan]

Our code review process is slow, and hard. I want to make PR review faster.

From my perspective, the current issue help maintainers review PR faster is not all related to the CI and it is only related to the PRs that are very large.

I think it is an answer to How to make big refactoring tasks or feature tasks?, we can add more integration tests and I agree, but in a large PR, it can not find all bugs too.

So the solution is that "separate the large PRs into small ones", it is a good way to resolve the help maintainers review PR faster issue in almost every projects.

And it is a suggestion to all contributors, if a feature/refactoring task is too big, consider creating an issue, and separating the tasks into an issue and add some descriptions on it, if the maintainers in community think it is good to go on, the tasks can get started to go on, if not, separate the tasks properly again according to the suggestions of maintainers. Take this one as an example #532, imagine that if I start only one PR to fix all tasks in this issue, it could drive reviewers crazy.

Maintainers should control the scale of a PR, if it is too big, it is hard to maintain, this is the only way to solve this issue.

Contributors and maintainers should keep a good communications by actively exchanging messages and ideas, it can prevent this issue deeply, not just rely on the CI, and make it too heavy by adding countless complex integration tests.

@mosn/layotto-commiter @mosn/layotto-pmc @mosn/layotto-admins @mosn/layotto-member How do you forks think?

I support this approach. The reviewer is indeed obliged to control the size of the pr to facilitate understanding and review. At the same time, I think ci can ensure the robustness of the entire code, but it will not be of much help for the review code. A pr of a suitable size, plus a simple demo is enough.

[seeflood]

If i want to run oss ci , i need provide my ak、sk in configure file？

@wenxuwan That's easy to solve because we can configurate it in the repo setting. I can help with it

[seeflood]

@Xunzhuo Thanks for your great suggestion. It makes sense! I followed your advice and split some big tasks into different subtasks, e.g. #471 #723

However..... I still think that "PR too large" is just one of the reasons that lead to the difficulty of review (of course, it's the most important reason) but there are also other reasons.

I think the reasons that make me feel hard to review include:

1. PR too large

2. No detailed design doc, and I can't understand the code. An example is https://github.com/mosn/layotto/pull/556#issuecomment-1179631138

3. PR didn't pass self-testing . If we use the analogy of a company's work, it's like "submitting a new feature to your Q&A colleague, but the Q&A guy sadly find that he can't run the feature successfully". It will take the Q&A guy much time asking the developer to fix all the issues and make it work. An example is #669 . I submitted several PR just to add tests for #669 and "prove" it doesn't work

Anyway, since you guys are not interested in adding integration tests, I'll compromise and give up this proposal :) At least I can help contributors add tests, e.g. https://github.com/mosn/layotto/pull/669#issuecomment-1177224860

[Xunzhuo]

2. No detailed design doc, and I can't understand the code. An example is feat: implement oss interface #556 (comment)

Definitely correct! We need the design docs, we should control the size of a PR and we should ask contributors to follow a good way to implement a feature or refactor task, after having an agreement on the design docs, we split tasks into small ones and get it started.

[Xunzhuo]

1. I support adding unit tests to feature codes.
2. And integration tests are useful indeed, but it should not be a demand for the feature implementor, it increases the difficult to start a task (If they are willing to do this, that is good, but if they think this is too complex, we should respect that). Instead, we can add a help-wanted issue to this integration test task.

[seeflood]

it increases the difficult to start a task

Yes, that's another reason I think I should give up this proposal. Asking too much will hurt the contributor experience 😢 As a new community, we should carefully protect the contributor experience. Adding a help-wanted issue is one way to go, the other way is adding tests by the reviewer (or me)

[Xunzhuo]

What a big PR (feature/refactor) should be processed?

1. A proposal with details to this task, which includes design and approach docs

1. Describe background to this task to tell us WHY we should do this.
2. How do we implement this feature, giving design docs and approachs.

Maintainers add comments/advice on this issue, and if no one have -1 for this task, move to step 2.

2. Split tasks into a few iterations if the process is too large. To figure out what we should implement first, second .... Rome was not built in a day. After that we can get started.

3. Keep a good communication with the maintainers while coding, maintainers and contributors both need to actively exchange messages and process, and questions can be discussed while implementing the task.

[Xunzhuo]

it increases the difficult to start a task

Yes, that's another reason I think I should give up this proposal. Asking too much will hurt the contributor experience 😢 As a new community, we should carefully protect the contributor experience. Adding a help-wanted issue is one way to go, the other way is adding tests by the reviewer (or me)

Yes both are Okay, if no one wants to take this issue, it should be added by maintainers.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity in the last 30 days. It will be closed in the next 7 days unless it is tagged (pinned, good first issue or help wanted) or other activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as pinned, good first issue or help wanted. Thank you for your contributions.