Page MenuHomePhabricator

Add pre-review shell script against git review hook to Translate extension
Closed, DeclinedPublic3 Estimated Story Points

Description

I think, to improve developer experience (especially for the new), it's important that a patch is not posted on gerrit before some checks are locally done. These checks would catch errors (formatting, lint, syntax etc) that would otherwise fail CI tests on Gerrit and eventually prolong the code review process.

This could be achieved by using a git hook to run a shell script with command that can be run on ones local development environment.

Event Timeline

Do shell scripts in Git hooks work on Windows?

Do shell scripts in Git hooks work on Windows?

Maybe 'shell scripts' in their entirety may be a stretch. We should be able to use Git's port of Bash on Windows. The script should be stores in .git/hooks. The hooks should be platform agnostic given that the checks to be made are dependent on Composer and PHP as far as I know. Checks to be run include:

composer fix
composer test
php phpunit 'path/to/unit'

For the unit tests, we can focus on the tests that changed or simply run all unit tests. Integration tests are not in scope due to the time it takes to run them.

More info

I am not a big fan of mandatory checks before pushing to Gerrit. That assumes I have a proper testing environment on the machine where I push, which is not the case for me where I push from a directory that is mounted inside virtual machine. I might be an edge case though.

Could the same be achieved with a local alias that runs those commands before submitting?

Checks to be run include:

composer fix
composer test
php phpunit 'path/to/unit'

For the unit tests, we can focus on the tests that changed or simply run all unit tests. Integration tests are not in scope due to the time it takes to run them.

It’s not only the time, integration tests also potentially modify the database, which is a development database, not a test database. Are you unit tests never modify the database?

I am not a big fan of mandatory checks before pushing to Gerrit. That assumes I have a proper testing environment on the machine where I push, which is not the case for me where I push from a directory that is mounted inside virtual machine. I might be an edge case though.

I don’t use a virtual machine, but I do often have a dirty working directory, i.e. I keep some changes uncommitted. (This happens less often in case of MediaWiki/Translate, as I do relatively little actual development, but I almost never have a clean working directory in repos I use in my IRL work.) So you’re an edge case, I’m also an edge, but adding all edge cases together, it may turn out that more people are “edge cases” than not. However,

Could the same be achieved with a local alias that runs those commands before submitting?

I fear not. As this task aims new developers anything that’s opt-in lowers the chances that it’ll be executed. Actually, composer fix && composer test isn’t much more difficult than composer precommit (or however it would be called), the difficult part is discovering it and remembering running it.

Looks like discussion has stopped without reaching a consensus. There is some opposition for mandatory hooks, but there might be other ways we can improve the developer experience.