Re: [PATCH v1] Documentation: add setup instructions for Travis CI

From: Lars Schneider <larsxschneider@gmail.com>
Date: Apr 16, 2016, 15:29 UTC
Message-ID: <E3568F7F-1C20-4B9F-BDF1-94A9DA08D5F7@gmail.com>
In-Reply-To: <xmqq4mb5l86f.fsf@gitster.mtv.corp.google.com>

On 13 Apr 2016, at 19:39, Junio C Hamano <gitster@pobox.com> wrote:

Show 26 quoted lines

> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> Documentation/SubmittingPatches | 39 ++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 36 insertions(+), 3 deletions(-)
>> 
>> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
>> index 98fc4cc..79e9b33 100644
>> --- a/Documentation/SubmittingPatches
>> +++ b/Documentation/SubmittingPatches
>> @@ -63,10 +63,43 @@ t/README for guidance.
>> When adding a new feature, make sure that you have new tests to show
>> the feature triggers the new behaviour when it should, and to show the
>> feature does not trigger when it shouldn't.  Also make sure that the
>> +test suite passes after your commit.
> 
> This is not a new issue, but it sounds as if you do not have to test
> if you are not doing a new shiny toy.  Perhaps we should rephrase
> the last sentence a bit.
> 
> 	After any code change, make sure that the entire test suite
> 	passes.  After any documentation change, make sure that the
> 	resulting documentation set formats well.

Agreed.

> By the way, can you teach our Travis thing to check for the "make
> doc" failures?

Yes, expect a patch soonish.

Show 12 quoted lines

> 
>> +We recommend to use our CI infrastructure to ensure your new feature
>> +passes all existing tests as well as your new tests on Linux, Mac, and
>> +(hopefully soon) Windows.  Follow these steps for the initial setup:
>> +
>> + (1) Sign in to GitHub: https://github.com
>> +     Please sign up first if you haven't already, it's free.
> 
> Three issues:
> 
> * None of the things utilized by the reader of this section looks
>   like "our" infrastructure to me.

OK. How about "We recommend to use the Travis CI infrastructure..." instead?

> * The above makes it sound as if we recommend everybody to become
>   GitHub customer, which is not really the case.

Agreed. I assume your suggested wording below would be fine?

> * This is overly long and deserves to be its own separate section,
>   just like we have MUA specific hints, this is GitHub-Travis
>   specific hints.

Agreed.

Show 24 quoted lines

> How about just saying
> 
> 	If you have an account at GitHub (and you can get one for
> 	free to work on open source projects), you can use their
> 	Travis CI integration to test your changes on Linux, Mac,
> 	and (hopefully soon) Windows.  See GitHub-Travis CI hints
> 	section for details.
> 	
> here, create a "GitHub-Travis CI hints" section just before "MUA
> specific hints" section, and move these numbered entries and the two
> paragraphs that follow to the new section.  As the introductory text
> for the new section itself, it may make sense to repeat a rephrased
> version of the above there, e.g.
> 
> 	--------------------------------------------------
>        GitHub-Travis CI hints
> 
> 	With an account at GitHub (you can get one for free to work
> 	on open source projects), you can use Travis CI to test your
> 	changes on Linux, Mac, and (hopefully soon) Windows.
> 
> 	Follow these steps for the initial setup:
> 
> 	(1) ...

Agreed. I also like Stefan's suggestion to move the CI stuff into a separate file. Any objections to this?

> I'd mildly prefer to leave "Please sign up first" line out
> of the first entry.

OK, I will remove it. My intention was to express that you need a GitHub account to use Travis CI.

Thanks for the review, Lars

Show 20 quoted lines

>> + ...
>> + (7) Enable Travis CI builds for your Git fork
>> +
>> +After the initial setup you can push your new feature branches to your
>> +Git fork on GitHub and check if they pass all tests here:
>> +https://travis-ci.org/<Your GitHub handle>/git/branches
>> +
>> +If they don't pass then they are marked "red". If that happens then
>> +click on the failing Travis CI job and scroll all the way down in the
>> +log. Find the line "<-- Click here to see detailed test output!" and
>> +click on the triangle next to the log line number to expand the detailed
>> +test output (example: https://travis-ci.org/git/git/jobs/122676187).
>> +Fix the problem and push an updated commit to your branch to ensure
>> +all tests pass.
>> +
>> +Do not forget to update the documentation to describe the updated
>> +behaviour of your new feature. It is currently a liberal mixture of US
>> and UK English norms for spelling and grammar, which is somewhat
>> unfortunate.  A huge patch that touches the files all over the place
>> only to correct the inconsistency is not welcome, though.  Potential