I dont think its unreasonable or unexpected in many cases that a test fails to compile without the changes in relates to. It depends what type of test it is and what the changes actually are though.
I agree it wont hugely change the PR though. Possibly why I prefer them being in the same commit is I tend to use the commit to look over things rather than the PR. On 19 April 2018 at 15:10, Clebert Suconic <[email protected]> wrote: > I have seen a few cases where the test would not compile.. that is the > test depends on the changes itself. What is not really Test Driven > Development. > > Both tests and fixes are part of the same PR.. so it doesn't really change > much. > > On Thu, Apr 19, 2018 at 9:41 AM, Robbie Gemmell > <[email protected]> wrote: >> In general I think having tests and changes in the same commit is >> nicer, especially for looking back at later. >> >> I'll also often apply a test on its own or revert the non-test changes >> to ensure tests fail, I've not really found it slow/annoying enough to >> specifically seperate tests out in their own commits to facilitate it. >> >> Robbie >> >> On 18 April 2018 at 18:27, Clebert Suconic <[email protected]> wrote: >>> I would appreciate if we separated fixes and tests on Pull Requests. >>> >>> >>> A lot of times i will revert the fixes to validate if the test is good >>> (if it fails without a fix) and how it failed. (not that I don't trust >>> the committer, just part of the validation as sometimes I want to see >>> what was the semantic change and fix). I may eventually play a better >>> fix in the process.. and I am sure that would apply to anyone else >>> helping on reviewing commits. >>> >>> >>> I had at some point gone back in history and needed to apply the test >>> without a fix to find a better fix. >>> >>> I know eventually it's not possible to separate these.. but if you >>> could as much as possible separate them:? >>> >>> >>> I recently did that into PR #2004... >>> >>> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585 >>> >>> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585 >>> >>> >>> >>> >>> I may update the hacking guide with this.. WDYT? >>> >>> >>> -- >>> Clebert Suconic > > > > -- > Clebert Suconic
