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 <[email protected]> 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 <[email protected] >> 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 <[email protected]> >> 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 <[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 >> > >> > >> > >> > -- >> > Tim Bish >> > twitter: @tabish121 >> > blog: http://timbish.blogspot.com/ >> > >> >> >> >> -- >> Clebert Suconic >> -- Clebert Suconic
