Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:73611
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.192.54 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <53404E5E.6010109@sugarcrm.com>
References: <53404E5E.6010109@sugarcrm.com>
Date: Sun, 6 Apr 2014 05:13:51 +0200
Message-ID: <CAEZPtU7YeFFkMt+KMb2Ef0-Z36cJbrZt9No5TWCUPc-=ew12kw@mail.gmail.com>
To: Stas Malyshev <smalyshev@sugarcrm.com>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Travis CI unit tests
From: pierre.php@gmail.com (Pierre Joye)

On Sat, Apr 5, 2014 at 8:41 PM, Stas Malyshev <smalyshev@sugarcrm.com> wrote:
> Hi!
>
> As many of us know, we have a setup on Travis CI which automatically
> runs unit tests for our active branches. This is very convenient for
> evaluating pull request and status of the branches and keeping us from
> making changes that break something without us noticing. However,
> there's a problem with this setup, and the problem is that CI build is
> never kept in the green for more than a a few days, the "normal" state
> of it is always failing. I think this situation is not normal and,
> frankly, embarrassing for the project, and propose to institute a policy
> to change it.
> The start would be recognizing that the goal is to keep the CI tests
> green. Once they are green, and somebody commits a change that makes
> them fail, we have the following options (note I'm not advocating for
> now any of them, just enumerating them) to fix it:
>
> 1. Revert the change immediately, and ask the submitter to fix the pull
> (pulls can be tested too on CI too) and re-submit it when it's green.
>
> 2. Wait for the change developer for a reasonable short time to fix the
> tests, if that does not happen, revert the change.
>
> 3. Put the failing tests into XFAIL until they are fixed by somebody.
>
> 4. Have somebody - e.g. RM for the branch - to modify the failing test
> to reflect the new results.


I am not a big fan of changing tests to make it work, especially for
stable branches. Except for warning&co additions, a test change means
a BC break. We have to be very careful here. Indeed I refer to
existing tests here, newly added tests to cover a bug fix is another
story, it can fail on one platform or a specific configuration. We can
fix them by making them configuration&system agnostic.

> These options, of course, can be used in combination and in
> case-per-case basis, but I think we need some explicit attention to the
> topic. I welcome everybody to propose suggestions about other options or
> ways we could fix this situation going forward and would like to hear
> the ideas about how we could make Travis CI tests useful and passing
> successfully, thus ensuring better quality in the project. After the
> initial discussion, I plan to write an RFC summarizing the proposals and
> have it voted on and, hopefully, accepted, so we'd have official policy
> on it, but fir now I'd also like to raise awareness of the issue and
> solicit ideas and participation of the members of the project on this topic.

One thing I would like to consider is peer reviews for patches in
stable branches. What you proposed goes in this direction, always go
through PR and travis before a change can be applied.

Cheers,
-- 
Pierre

@pierrejoye | http://www.libgd.org