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/
