[DISCUSS] Separate commit for Test and Fix on PRs

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[DISCUSS] Separate commit for Test and Fix on PRs

clebertsuconic
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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Separate commit for Test and Fix on PRs

MichaelAndrePearce
The only comment here is we will need to stop squashing our commits in general eg be happy a pr may have many commits I’m happy to do this btw, eg internally in our org we don’t care in fact we prefer the full devs commits as then it helps unpick stuff, I found it a shock to be squashing so much when I started.

but generally when locally working I may commit multiple times a day different bits eg rework the fix or enhance the test further, it’s one of the big benefits of git for me, but before push and PR I squash the commits as up till now as we don’t seem to like to have this history.



Sent from my iPhone

> On 18 Apr 2018, at 18:27, Clebert Suconic <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Separate commit for Test and Fix on PRs

clebertsuconic
I would prefer a squash before merging.. or at least some cleanup
before people submit a PR.

When reviewing a PR.. it's almost impossible to make a correlation
between the committs...


example commit1: added a line, commit 2: removed that same line... it
would make a PR review useless and more hard.


I'm trying to make the review of PRs easier.. having a full day of
work being sent on a PR as a stream will make the reviewer go through
what you tried along the day.


After all. you should be able to checkout and compile any PR.




Anyways: I'm not asking to add the test separated as a strict rule. if
for some reason it became difficult to do it along your PR.. that's
fine. (which this will of course only apply for bug fixes).





On Wed, Apr 18, 2018 at 3:34 PM, Michael André Pearce
<[hidden email]> wrote:

> The only comment here is we will need to stop squashing our commits in general eg be happy a pr may have many commits I’m happy to do this btw, eg internally in our org we don’t care in fact we prefer the full devs commits as then it helps unpick stuff, I found it a shock to be squashing so much when I started.
>
> but generally when locally working I may commit multiple times a day different bits eg rework the fix or enhance the test further, it’s one of the big benefits of git for me, but before push and PR I squash the commits as up till now as we don’t seem to like to have this history.
>
>
>
> Sent from my iPhone
>
>> On 18 Apr 2018, at 18:27, Clebert Suconic <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Separate commit for Test and Fix on PRs

clebertsuconic
You can cleanup your commits with a git rebase -i HEAD~<NUMBER-OF-COMMITTS>

Squash them accordingly... etc... it's an easy operation.

Complementing what I said earlier... Many committs are ok.. as long as
they are semantically correct... Intermediate committs that you did
along your day should be cleaned up and squashed. that makes reviewing
a lot easier.


Even if you are committing directly upstream (you still have that
right as a committer) I would ask you to review your committs before
pushing. We like to have the github PRs as a peer review tool.. and
quite honestly it has stopped even myself from doing damage before. I
like PRs just because of that.



On Wed, Apr 18, 2018 at 3:54 PM, Clebert Suconic
<[hidden email]> wrote:

> I would prefer a squash before merging.. or at least some cleanup
> before people submit a PR.
>
> When reviewing a PR.. it's almost impossible to make a correlation
> between the committs...
>
>
> example commit1: added a line, commit 2: removed that same line... it
> would make a PR review useless and more hard.
>
>
> I'm trying to make the review of PRs easier.. having a full day of
> work being sent on a PR as a stream will make the reviewer go through
> what you tried along the day.
>
>
> After all. you should be able to checkout and compile any PR.
>
>
>
>
> Anyways: I'm not asking to add the test separated as a strict rule. if
> for some reason it became difficult to do it along your PR.. that's
> fine. (which this will of course only apply for bug fixes).
>
>
>
>
>
> On Wed, Apr 18, 2018 at 3:34 PM, Michael André Pearce
> <[hidden email]> wrote:
>> The only comment here is we will need to stop squashing our commits in general eg be happy a pr may have many commits I’m happy to do this btw, eg internally in our org we don’t care in fact we prefer the full devs commits as then it helps unpick stuff, I found it a shock to be squashing so much when I started.
>>
>> but generally when locally working I may commit multiple times a day different bits eg rework the fix or enhance the test further, it’s one of the big benefits of git for me, but before push and PR I squash the commits as up till now as we don’t seem to like to have this history.
>>
>>
>>
>> Sent from my iPhone
>>
>>> On 18 Apr 2018, at 18:27, Clebert Suconic <[hidden email]> 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



--
Clebert Suconic
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Separate commit for Test and Fix on PRs

MichaelAndrePearce
+1 especially as you’ve explained it will be guidance and not a hard and fast rule.

Sent from my iPhone

> On 18 Apr 2018, at 21:06, Clebert Suconic <[hidden email]> wrote:
>
> You can cleanup your commits with a git rebase -i HEAD~<NUMBER-OF-COMMITTS>
>
> Squash them accordingly... etc... it's an easy operation.
>
> Complementing what I said earlier... Many committs are ok.. as long as
> they are semantically correct... Intermediate committs that you did
> along your day should be cleaned up and squashed. that makes reviewing
> a lot easier.
>
>
> Even if you are committing directly upstream (you still have that
> right as a committer) I would ask you to review your committs before
> pushing. We like to have the github PRs as a peer review tool.. and
> quite honestly it has stopped even myself from doing damage before. I
> like PRs just because of that.
>
>
>
> On Wed, Apr 18, 2018 at 3:54 PM, Clebert Suconic
> <[hidden email]> wrote:
>> I would prefer a squash before merging.. or at least some cleanup
>> before people submit a PR.
>>
>> When reviewing a PR.. it's almost impossible to make a correlation
>> between the committs...
>>
>>
>> example commit1: added a line, commit 2: removed that same line... it
>> would make a PR review useless and more hard.
>>
>>
>> I'm trying to make the review of PRs easier.. having a full day of
>> work being sent on a PR as a stream will make the reviewer go through
>> what you tried along the day.
>>
>>
>> After all. you should be able to checkout and compile any PR.
>>
>>
>>
>>
>> Anyways: I'm not asking to add the test separated as a strict rule. if
>> for some reason it became difficult to do it along your PR.. that's
>> fine. (which this will of course only apply for bug fixes).
>>
>>
>>
>>
>>
>> On Wed, Apr 18, 2018 at 3:34 PM, Michael André Pearce
>> <[hidden email]> wrote:
>>> The only comment here is we will need to stop squashing our commits in general eg be happy a pr may have many commits I’m happy to do this btw, eg internally in our org we don’t care in fact we prefer the full devs commits as then it helps unpick stuff, I found it a shock to be squashing so much when I started.
>>>
>>> but generally when locally working I may commit multiple times a day different bits eg rework the fix or enhance the test further, it’s one of the big benefits of git for me, but before push and PR I squash the commits as up till now as we don’t seem to like to have this history.
>>>
>>>
>>>
>>> Sent from my iPhone
>>>
>>>> On 18 Apr 2018, at 18:27, Clebert Suconic <[hidden email]> 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
>
>
>
> --
> Clebert Suconic
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Separate commit for Test and Fix on PRs

Robbie Gemmell
In reply to this post by clebertsuconic
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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Separate commit for Test and Fix on PRs

clebertsuconic
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
<[hidden email]> 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Separate commit for Test and Fix on PRs

Robbie Gemmell
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 <[hidden email]> 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
> <[hidden email]> 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Separate commit for Test and Fix on PRs

tabish121@gmail.com
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 <[hidden email]> 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
>> <[hidden email]> 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 <[hidden email]> 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/

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Separate commit for Test and Fix on PRs

clebertsuconic
will I sound crazy if I change my mind here?

I thought it would be improving the PR process.. but on a second
thought... it won't improve things actually and i will agree with
Robbie here.

 So. .just ignore me.. and apologize for spamming  you guys..

On Thu, Apr 19, 2018 at 10:48 AM, Timothy Bish <[hidden email]> wrote:

> 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 <[hidden email]>
>> 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
>>> <[hidden email]> 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 <[hidden email]>
>>>> 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/
>



--
Clebert Suconic
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Separate commit for Test and Fix on PRs

Martyn Taylor
I think having test that don't compile due to code changes is an
exception.  I think we should strive for this approach.  Clebert, I follow
the same process you described, I undo the change then run the test to see
what was wrong and that the test is valid.  In my opinion this is a
positive thing to strive to.  Obviously common sense should prevail, and if
there are some reason it's not possible or very difficult then we can relax
the rule.

On Thu, Apr 19, 2018 at 10:13 PM, Clebert Suconic <[hidden email]
> wrote:

> will I sound crazy if I change my mind here?
>
> I thought it would be improving the PR process.. but on a second
> thought... it won't improve things actually and i will agree with
> Robbie here.
>
>  So. .just ignore me.. and apologize for spamming  you guys..
>
> On Thu, Apr 19, 2018 at 10:48 AM, Timothy Bish <[hidden email]>
> wrote:
> > 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 <[hidden email]>
> >> 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
> >>> <[hidden email]> 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 <[hidden email]
> >
> >>>> 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/
> >
>
>
>
> --
> Clebert Suconic
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Separate commit for Test and Fix on PRs

clebertsuconic
Another reason to not do this is reporting.

I have implemented a report tool to go over the commits, and report
code changes versus tests added. Having separate commits would make
such analysis more difficult.

so, again Clebert Today against Clebert last week.


In case you want to check it out:
https://github.com/clebertsuconic/git-release-report

It can produce some nice reporting.. example:
https://gist.githubusercontent.com/clebertsuconic/4289196e2e45c74dc8fef8fad5d92fe3/raw/dab4dbca344d402f9e1775fe1605a919e9d07163/screen-shot.png


https://gist.github.com/clebertsuconic/4289196e2e45c74dc8fef8fad5d92fe3#file-report-html




On Fri, Apr 20, 2018 at 6:46 AM, Martyn Taylor <[hidden email]> wrote:

> I think having test that don't compile due to code changes is an
> exception.  I think we should strive for this approach.  Clebert, I follow
> the same process you described, I undo the change then run the test to see
> what was wrong and that the test is valid.  In my opinion this is a
> positive thing to strive to.  Obviously common sense should prevail, and if
> there are some reason it's not possible or very difficult then we can relax
> the rule.
>
> On Thu, Apr 19, 2018 at 10:13 PM, Clebert Suconic <[hidden email]
>> wrote:
>
>> will I sound crazy if I change my mind here?
>>
>> I thought it would be improving the PR process.. but on a second
>> thought... it won't improve things actually and i will agree with
>> Robbie here.
>>
>>  So. .just ignore me.. and apologize for spamming  you guys..
>>
>> On Thu, Apr 19, 2018 at 10:48 AM, Timothy Bish <[hidden email]>
>> wrote:
>> > 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 <[hidden email]>
>> >> 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
>> >>> <[hidden email]> 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 <[hidden email]
>> >
>> >>>> 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/
>> >
>>
>>
>>
>> --
>> Clebert Suconic
>>



--
Clebert Suconic