プルリクエストのレビュー

プロジェクトには多くのプルリクエストがプッシュされており、それらをレビューしたりテストしたりすることでコアチームを支援することができます。

このWikiでは、この作業をどのように支援するかを説明したいと思います。

なぜこれが重要なのか?
2020年4月以降、ブルリクエストはプロジェクトへのコードの貢献のデフォルトの方法(Fork and Branch Git ワークフロー)となっています。ご存知のように、iDempiereはオープンソースのソフトウェアプロジェクトであり、コードはプロジェクトの最も重要な部分です。これらのバグ修正や機能追加は、コードが適切に書かれていれば、コミュニティの全員に利益をもたらします。

iDempiereのリーダーたちは、最高品質のコードを維持することを重視しています。彼らは、トランクに入るものや変更すべきものをレビューし、フィルタリングすることで、これを実現しています。問題は、プロジェクトが多くのリクエストを受ければ受けるほど、キューが長くなり、時間の経過とともに迷走するリクエストが出てくることです。

プルリクエストをレビューすることで、最初のフィルタになることができ、適切に実行すれば、リーダーの肩から多くの作業を省くことができ、重要なコードをより早くトランクに入れることができます。

プルリクエストをレビューするには?

コードがトランクに入る前にレビューしなければならないことはたくさんありますが、大まかには2つにまとめることができます。

1. コードはきちんと書かれていて、付随的な損害やセキュリティ侵害をもたらさないか？
2. コードは意図した通りの動作をしているか？

ローカル環境に何も足さなくてもできることは、まずコードを見直すことです。

コードレビュー

• Choose one of the open pull requests.
• Open the list of commits done against the pull request to see the changes in the code that were done by the developer.
• Check that the submitted code complies with the project guidelines to contribute to the trunk. The main points to consider are:
  • It has to work in PostgreSQL and Oracle.
  • When possible avoid the use of SQL Code, use the data model instead.
  • If changes in the database were made, the migration scripts are present.
  • JDBC should be avoided if it is used because there is no other way, every cursor must be closed in a "finally" block.
  • All the code is written in English.
  • The pull request does not mix purposes (f.i refactoring and new feature) and changes only the necessary classes to address the ticket's goal.

Once you validate that the code is well written and satisfies the project's guidelines and best practices, proceed to pull the code to your local environment to test.

Review Functionality

In your local environment run:

git checkout master
git pull upstream master
git push
git pull --no-commit https://github.com/(pullrequest-github-user)/idempiere.git IDEMPIERE-(Jira-ticket-number) (for example: git pull --no-commit https://github.com/globalqss/idempiere.git IDEMPIERE-1848) 1

1 Sometimes even when you specify --no-commit, git adds the commit to your log. If that is the case, check with git log and run git reset --soft HEAD~ to remove the commit from the log and keep your tree clean.

Once you have the changes in your local environment.

1. Apply the migration scripts if necessary.
2. Run iDempiere and TEST.
3. Test every possible scenario, do not only test what was intended to be solved with the ticket but verify also that previous functionality is not affected by the commit. The system should behave as before except for the new addition.

Revert everything to keep master clean.

After you've performed your review, report your findings on the Jira ticket, or add a comment directly to the pull request.

You can be as specific as possible, describing what you've tested and the results.

If you found any issues, comment on what you've found and how you think it could be solved if you have any suggestions.

If the results are positive, comment that you've tested the ticket and you find the solution to be OK.

TL;DR;

1. Choose one of the open pull requests.
2. Do a code review based on the project's guidelines to contribute to the trunk.
3. Test the ticket's functionality.
4. Make sure there is no collateral damage or security breaches caused by the ticket.
5. Report your findings on JIRA or directly to the pull request.

I recommend you to test thoughtfully and be completely honest about what you did and what you didn't do. This is an open-source community, you have many eyes on you and your reputation will benefit or get hurt depending on what you do.