activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Timothy Bish <tabish...@gmail.com>
Subject Re: [DISCUSS] Separate commit for Test and Fix on PRs
Date Thu, 19 Apr 2018 14:48:33 GMT
On 04/19/2018 10:32 AM, Robbie Gemmell wrote:
> 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.

The other thing to keep in mind is that when two or more commits for the 
same bit of work go in, the process of reverting changes becomes more 
error prone as the person doing the reverts has to always be looking for 
the cases where there was more than one related to the same scope of work.

> On 19 April 2018 at 15:10, Clebert Suconic <clebert.suconic@gmail.com> 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
>> <robbie.gemmell@gmail.com> 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 <clebert.suconic@gmail.com>
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


-- 
Tim Bish
twitter: @tabish121
blog: http://timbish.blogspot.com/


Mime
View raw message