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

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

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

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

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

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

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

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

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

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

コードレビュー

• オープンになっているプルリクエストから、１つを選んで下さい。
• プルリクエストに対して行われたコミットのリストを開くと、開発者によって行われたコードの変更を見ることができます。
• Check that the submitted code complies with the project guidelines to contribute to the trunk. The main points to consider are:
  • PostgreSQL と Oracle の両方で動作しなければなりません。
  • 可能な限りSQLコードの使用を避け、代わりにモデルクラスを使用してください。
  • データベースに変更があった場合は、移行スクリプトが存在します。
  • DBにSQLでアクセスする際には、すべてのカーソルは"finally"句で、閉じなければなりません。
  • コードはすべて英語で書かれています。
  • プルリクエストは目的(例えば、リファクタリングと新機能など)を混在させることなく、チケットの目的に対応するために必要なクラスのみを変更します。

コードがよく書かれていること、プロジェクトのガイドラインやベストプラクティスを満たしていることを確認したら、ローカル環境にコードを引っ張り出してテストします。

機能のレビュー

あなたのローカル環境で実行して下さい。:

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 --no-commit を指定していても、git がコミットをログに追加してしまうことがあります。そのような場合は、git のログを確認して git reset --soft HEAD~ を実行すると、ログからコミットを削除してツリーをきれいに保つことができます。

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.