[VOTE] CI tests RFC

Chris Wright

12 years ago

Hi Stas

On 25 April 2014 08:50, Stas Malyshev <smalyshev@sugarcrm.com> wrote:

> Hi!
>
> I'd like to propose the CI tests RFC for the vote. Please vote at:
> https://wiki.php.net/rfc/travis_ci
>
> The vote is planned to close on May 5. If you have any questions, please
> contact me.

The 4th option for the "how to handle" vote doesn't seem to gel with
the other 3. It pertains mostly to what's probably a minority of
contributions, where the expected behaviour for an existing test is
altered - i.e. a BC break (these cases should have been thoroughly
examined by the community and the failing tests should have been
caught before this point, but that's another story).

The other 3 describe generic course of actions that could be applied
to any change that breaks the CI tests.

It seems to me that this option is actually a 3rd yes/no vote, namely
"should the RM be responsible for inspecting the failing tests and
determining whether code is wrong or the test is wrong", so we would
have:

Vote 1 - general acceptance (y/n)
Vote 2 - should RM be responsible for looking at failed tests (y/n)
Vote 3 - preferred action when tests are failing

If vote 2 has a majority "yes", then the RM would see if the test
needs to be updated, and then fall down to the selected option from
vote 3. If vote 2 is a majority "no", then the vote 3 result would be
applied in all cases, and it would be the contributor's responsibility
to find the case where the test needs updating.

Thoughts?

Best Regards,

Chris Wright

Ferenc Kovacs

12 years ago

On Fri, Apr 25, 2014 at 9:50 AM, Stas Malyshev <smalyshev@sugarcrm.com>wrote:

> Hi!
>
> I'd like to propose the CI tests RFC for the vote. Please vote at:
> https://wiki.php.net/rfc/travis_ci
>
> The vote is planned to close on May 5. If you have any questions, please
> contact me.
> --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>

Would be nice if we could also create some policy regarding the usage of
XFAILs (when should be a failing test marked as XFAIL, should we add the
tests from the open bugreports as XFAIL by default, whose responsibility is
to make sure that it will be fixed eventually, etc.).
I would also like to extend the current travis config a bit (we could have
more exts, more axes for stuff like ts/nts builds, enable debug builds, so
memory leaks are also triggering the test failures, etc.), which could on
the short term cause some "new" test failures (but also give those problems
more visibility, hence better chances to be fixed).

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

Stas Malyshev

12 years ago

Hi!

> Would be nice if we could also create some policy regarding the usage of
> XFAILs (when should be a failing test marked as XFAIL, should we add the
> tests from the open bugreports as XFAIL by default, whose responsibility
> is to make sure that it will be fixed eventually, etc.).

I agree. I didn't get into this but it definitely makes sense to have
some rules there.

> I would also like to extend the current travis config a bit (we could
> have more exts, more axes for stuff like ts/nts builds, enable debug
> builds, so memory leaks are also triggering the test failures, etc.),

You're more than welcome :) I've planned to get to some of it next -
i.e. going through the list of exts and see which ones we can support on
Travis - but any help would be great.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Joe Watkins

12 years ago

On Fri, 2014-04-25 at 11:41 -0700, Stas Malyshev wrote:

> Hi!
>
> > Would be nice if we could also create some policy regarding the usage of
> > XFAILs (when should be a failing test marked as XFAIL, should we add the
> > tests from the open bugreports as XFAIL by default, whose responsibility
> > is to make sure that it will be fixed eventually, etc.).
>
> I agree. I didn't get into this but it definitely makes sense to have
> some rules there.
>
> > I would also like to extend the current travis config a bit (we could
> > have more exts, more axes for stuff like ts/nts builds, enable debug
> > builds, so memory leaks are also triggering the test failures, etc.),
>
> You're more than welcome :) I've planned to get to some of it next -
> i.e. going through the list of exts and see which ones we can support on
> Travis - but any help would be great.
>
>
> --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227
>

Morning Stas,

I got time to read through this morning properly, obviously, yes ...
but I'm not sure about the options, maybe I missed some relevant
conversation around it, not sure ... a few questions quick before I
finish voting if you don't mind ...

Update test seems like questionable action, but see that you and mike
voted for it, can I hear reasoning for that ?

I don't see when there would be a good reason to retain a broken commit
for a week, isn't that going to just render all of this pointless if
tests can fail for a week at a time ?

Wouldn't it be better if all changes were done on branches, so they can
be reviewed and integrated before being merged at all ?

Maybe problematic for minor fixes but are they really the kind of thing
that cause CI to bork ?

Cheers
Joe

Stas Malyshev

12 years ago

Hi!

> 	Update test seems like questionable action, but see that you and mike
> voted for it, can I hear reasoning for that ?

Some tests may test for wrong outcome. Simplest example - you've added
an item for function returning an array, but did not update some test to
include it. Or, you've fixed a bug but some test had the buggy output
included in the test.

> 	I don't see when there would be a good reason to retain a broken commit
> for a week, isn't that going to just render all of this pointless if
> tests can fail for a week at a time ?

I agree. But this is an option, so I put it up for the vote. If most of
the people think week is too long (and we've had many instance where the
CI was red for more than a week, so this is not an invented scenario)
I'd be glad to hear it and we'd have it officially acknowledged.

>
> 	Wouldn't it be better if all changes were done on branches, so they can
> be reviewed and integrated before being merged at all ?

Of course, it would. That's why changes should be done in pulls, and
pulls should be green before merging. However, it happens that both
rules are not followed, and that even if they are, for some reason the
merge is still not green. And in general, I don't think we can enforce
anything in this regard until we know we agree on this subject.

> 	Maybe problematic for minor fixes but are they really the kind of thing
> that cause CI to bork ?

Hopefully not. If the fix breaks the CI and can not be fixed in a short
time - then maybe the fix is not as minor as previously thought :)

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Ferenc Kovacs

12 years ago

On Sat, Apr 26, 2014 at 8:51 AM, Joe Watkins <pthreads@pthreads.org> wrote:

> On Fri, 2014-04-25 at 11:41 -0700, Stas Malyshev wrote:
> > Hi!
> >
> > > Would be nice if we could also create some policy regarding the usage
> of
> > > XFAILs (when should be a failing test marked as XFAIL, should we add
> the
> > > tests from the open bugreports as XFAIL by default, whose
> responsibility
> > > is to make sure that it will be fixed eventually, etc.).
> >
> > I agree. I didn't get into this but it definitely makes sense to have
> > some rules there.
> >
> > > I would also like to extend the current travis config a bit (we could
> > > have more exts, more axes for stuff like ts/nts builds, enable debug
> > > builds, so memory leaks are also triggering the test failures, etc.),
> >
> > You're more than welcome :) I've planned to get to some of it next -
> > i.e. going through the list of exts and see which ones we can support on
> > Travis - but any help would be great.
> >
> >
> > --
> > Stanislav Malyshev, Software Architect
> > SugarCRM: http://www.sugarcrm.com/
> > (408)454-6900 ext. 227
> >
>
> Morning Stas,
>
>         I got time to read through this morning properly, obviously, yes
> ...
> but I'm not sure about the options, maybe I missed some relevant
> conversation around it, not sure ... a few questions quick before I
> finish voting if you don't mind ...
>
>         Update test seems like questionable action, but see that you and
> mike
> voted for it, can I hear reasoning for that ?
>
>         I don't see when there would be a good reason to retain a broken
> commit
> for a week, isn't that going to just render all of this pointless if
> tests can fail for a week at a time ?
>
>         Wouldn't it be better if all changes were done on branches, so
> they can
> be reviewed and integrated before being merged at all ?
>
>         Maybe problematic for minor fixes but are they really the kind of
> thing
> that cause CI to bork ?
>
> Cheers
> Joe
>
>

I also agree that update test is a questionable option there.
If we are talking about fixing a bad test, then it is obvious, and
shouldn't be even put up for a vote, as naturally everybody including the
RMs are free to fix the broken tests.
So the only other reason listing that option is when somebody intentionally
or unintentionally changes the behavior of some code without updating the
tests.
If that happens without proper discussion or RFC, I would say that it would
be mandatory to revert such changes until consensus/approval of that can be
made.

tl;dr: fixing the wrong test is always an option, but to fix up the test to
cover a bad code change shouldn't be an option.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

Ferenc Kovacs

12 years ago

On Fri, Apr 25, 2014 at 8:41 PM, Stas Malyshev <smalyshev@sugarcrm.com>wrote:

> Hi!
>
> > Would be nice if we could also create some policy regarding the usage of
> > XFAILs (when should be a failing test marked as XFAIL, should we add the
> > tests from the open bugreports as XFAIL by default, whose responsibility
> > is to make sure that it will be fixed eventually, etc.).
>
> I agree. I didn't get into this but it definitely makes sense to have
> some rules there.
>

yeah, I'm glad that we managed to reach a state where we have
0/close-to-zero number of failing tests for the default builds on the major
platforms, but the number of XFAILS are somewhat scarry, and some of there
are pretty old too (there are a couple of them which I remember discussed
to be fixed before 5.4.0 final...).
the reason why we introduced the XFAILs were to be able to distinguish
between the old/known/low-priority problems from new/important test
failures.
but now if we want to keep the failing test numbers on zero, sooner or
later people will start putting XFAILs on everything to satisfy the CI, and
we will end up in the same situation where the less important XFAILS will
cause the important ones to slip under the radar.

>
> > I would also like to extend the current travis config a bit (we could
> > have more exts, more axes for stuff like ts/nts builds, enable debug
> > builds, so memory leaks are also triggering the test failures, etc.),
>
> You're more than welcome :) I've planned to get to some of it next -
> i.e. going through the list of exts and see which ones we can support on
> Travis - but any help would be great.
>

I've created a PR with some travis related changes:
https://github.com/php/php-src/pull/654

I've also looked into having both 32 and 64 bit builds, but it seems travis
only supports 64bit platforms atm:
https://github.com/travis-ci/travis-ci/issues/986

and also looked into enabling the email notifications, but it seems that it
is a bit complicated:
http://docs.travis-ci.com/user/notifications/#Email-notifications
the default behavior is:
"By default, email notifications will be sent to the committer and the
commit author, if they are members of the repository (that is, they have
push or admin permissions for public repositories, or if they have pull,
push or admin permissions for private repositories).
...
By default, a build email is sent to the committer and the author, but only
if they have access to the repository the commit was pushed to. This
prevents forks active on Travis CI from notifying the upstream repository's
owners when they're pushing any upstream changes to their fork. It also
prevents build notifications from going to folks not registered on Travis
CI.
...
The email address is then determined based on the email address in the
commit, but only if it matches one of the email addresses in our database.
We synchronize all your email addresses from GitHub, solely for the purpose
of build notifications."

which we can't really use, because almost nobody has push/admin rights to
our github repo, and adding an explicit email address (like internals@ or
php-qa@) instead of using the default strategy would mean that any fork
which get's activated on travis will start spamming those lists. :/

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

Pierre Joye

12 years ago

hi Stas,

On Fri, Apr 25, 2014 at 9:50 AM, Stas Malyshev <smalyshev@sugarcrm.com> wrote:

> Hi!
>
> I'd like to propose the CI tests RFC for the vote. Please vote at:
> https://wiki.php.net/rfc/travis_ci
>
> The vote is planned to close on May 5. If you have any questions, please
> contact me.

I voted yes, obviously.

However there are one thing I like to add, or other RFCs, the "fix
test to work with patch XYZ" thing sounds bad in general for stable
branches. Usually bc breaks. We may need more clarity here.

Sorry to do not have talked about that before voting :/

Cheers,

--
Pierre

@pierrejoye | http://www.libgd.org

Stas Malyshev

12 years ago

Hi!

> However there are one thing I like to add, or other RFCs, the "fix
> test to work with patch XYZ" thing sounds bad in general for stable
> branches. Usually bc breaks. We may need more clarity here.

Well, in most cases, yes, but we've had instances where tests were
testing for buggy results, because "this is how it works now". In any
case, this is exactly why I put the options to vote - so people could
express their opinions about them. Of course, for each specific case it
has to be judged on specific basis, one-size-fits-all is not good here.
I just wanted to see how people in general feel about each option.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Derick Rethans

12 years ago

On Fri, 25 Apr 2014, Stas Malyshev wrote:

> I'd like to propose the CI tests RFC for the vote. Please vote at:
> https://wiki.php.net/rfc/travis_ci
>
> The vote is planned to close on May 5. If you have any questions,
> please contact me.

I have a comment (not specifically to you). We can't seriously be
suggesting that RMs can just revert commits. This is a really rude thing
to do in an open source project. We're doing this for fun, and people
immediately reverting your commits takes (more) fun out of it.

cheers,
Derick

Pierre Joye

12 years ago

hi,

On Sat, Apr 26, 2014 at 11:38 AM, Derick Rethans <derick@php.net> wrote:

> On Fri, 25 Apr 2014, Stas Malyshev wrote:
>
>> I'd like to propose the CI tests RFC for the vote. Please vote at:
>> https://wiki.php.net/rfc/travis_ci
>>
>> The vote is planned to close on May 5. If you have any questions,
>> please contact me.
>
> I have a comment (not specifically to you). We can't seriously be
> suggesting that RMs can just revert commits. This is a really rude thing
> to do in an open source project. We're doing this for fun, and people
> immediately reverting your commits takes (more) fun out of it.

- review
- ask for fix
- revert

I do not see what is rude here. However not fixing a patch within a
couple of days (as in commit and run), after other has reported
issues, is actually rude.

This procedure should have happened long time ago already, at least
for stable branches. I would go even one step further, only allow RMs
to cherry pick patches and let devs commit to dev branches, be master
or next patches release for stable dev branches. Doing so will allow
much faster security releases for example and avoid last minutes
patches breaking a release.

Cheers,

--
Pierre

@pierrejoye | http://www.libgd.org

Sebastian Bergmann

12 years ago

Am 26.04.2014 11:57, schrieb Pierre Joye:

> - review
> - ask for fix
> - revert
>
> I do not see what is rude here. However not fixing a patch within a
> couple of days (as in commit and run), after other has reported
> issues, is actually rude.

Agreed; committing or submitting a patch without making sure that it
does not break any tests is rude.

Stas Malyshev

12 years ago

Hi!

> I have a comment (not specifically to you). We can't seriously be
> suggesting that RMs can just revert commits. This is a really rude thing
> to do in an open source project. We're doing this for fun, and people
> immediately reverting your commits takes (more) fun out of it.

I agree. That's why we have time limits before that should happen.
However, it has happened that people made commits which break things and
then have gone unresponsive for an extended period of time, with broken
commits laying there and making everybody work much harder to ensure
breakage does not spread (if CI is red, then any pull against it is red,
so we can't really trust the pulls tests, and when we merge them we
don't know anymore what exactly broke it and it becomes a mess very
quickly). So I think the normal workflow would be as follows:

1. Make pull
2. See the pull test green
3. Merge the pull
4. Ensure the CI for main branch is still green
5. Go have a beer/coffee/well-deserved rest

However, if somebody commits something that breaks the CI, and is not
fixing it, we need to know it's OK to fix it, and we need the committer
to know too if he's negligent about CI hygiene his commit may not be
accepted.
If there's an exceptional situation - e.g. somebody breaks the CI but
knows how to fix it but can do it only in 3 days because of stuff, he
can always write to the list saying so and RM should make an exception.
What we're trying to prevent is committer not reacting and everybody
sitting around doing nothing and CI being red for weeks.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Julien Pauli

12 years ago

On Sun, Apr 27, 2014 at 9:44 PM, Stas Malyshev <smalyshev@sugarcrm.com> wrote:

> Hi!
>
>> I have a comment (not specifically to you). We can't seriously be
>> suggesting that RMs can just revert commits. This is a really rude thing
>> to do in an open source project. We're doing this for fun, and people
>> immediately reverting your commits takes (more) fun out of it.
>
> I agree. That's why we have time limits before that should happen.
> However, it has happened that people made commits which break things and
> then have gone unresponsive for an extended period of time, with broken
> commits laying there and making everybody work much harder to ensure
> breakage does not spread (if CI is red, then any pull against it is red,
> so we can't really trust the pulls tests, and when we merge them we
> don't know anymore what exactly broke it and it becomes a mess very
> quickly). So I think the normal workflow would be as follows:
>
> 1. Make pull
> 2. See the pull test green
> 3. Merge the pull
> 4. Ensure the CI for main branch is still green
> 5. Go have a beer/coffee/well-deserved rest

No offense, but I've kept thinking it was the case till now.

Hang on, talking about my own 5.5 RM experience, and knowing we are
talking about an open source project, I always privilege the
discussion when there is some conflict.
It has happened that I had failing tests in a release branch, then I
go thought git history and try to contact the guy behind this.
If no answer, there is no other choice than reverting (or trying to
fix the case, which often is not an option as it may be hard).

I agree that the default case should always be to have a green PR. In
fact, I cant understand someone trying to PR something turning tests
to red, that's just over my mind, but if you say that happens, OK so.

I would tend to say that if you red the tests, you'll never be merged.
Someone still can contact the RM in case of any problem, but guy, if
you put tests to red, if you think your work is not finished yet
(weither you have time to finish it or not) : just don't ask for PR.
The only thing it will bring is confusion to other contributors, and
RM fighting with your commits and probably with you as well :-P

I don't really like exceptions. If you turn the tests to red and say
you have no time for now to green them back, then why not just wait
nicely in your feature branch, and we'll merge it later ? Why rush ?

Git is wonderful at collaboration, there is no need to rush to force a
PR to be merged knowing it will fail tests and introduce some bad code
in a stable branch, that is just not acceptable.

Regarding XFails, I have no advice about them. I dont really know why
they are here etc...

Having said that, I agree with Ferenc saying we have very uncommon
situations where some tests fail for stable releases, which is a very
good point.

Julien

Derick Rethans

12 years ago

On Sun, 27 Apr 2014, Stas Malyshev wrote:

> Hi!
>
> > I have a comment (not specifically to you). We can't seriously be
> > suggesting that RMs can just revert commits. This is a really rude thing
> > to do in an open source project. We're doing this for fun, and people
> > immediately reverting your commits takes (more) fun out of it.
>
> I agree. That's why we have time limits before that should happen.
> However, it has happened that people made commits which break things and
> then have gone unresponsive for an extended period of time, with broken
> commits laying there and making everybody work much harder to ensure
> breakage does not spread (if CI is red, then any pull against it is red,
> so we can't really trust the pulls tests, and when we merge them we
> don't know anymore what exactly broke it and it becomes a mess very
> quickly). So I think the normal workflow would be as follows:
>
> 1. Make pull
> 2. See the pull test green
> 3. Merge the pull
> 4. Ensure the CI for main branch is still green
> 5. Go have a beer/coffee/well-deserved rest

Maybe, but for this to work you need to teach everybody proper git
workflows. In a project with many infrequent committers, you're never
going to get this done. Heck, it can be hard in a 3 man team to have
proper git discipline.

> However, if somebody commits something that breaks the CI, and is not
> fixing it, we need to know it's OK to fix it, and we need the
> committer to know too if he's negligent about CI hygiene his commit
> may not be accepted.

Then why do you have as an option in your voting "Revert immediately"?
That should never be happening.

cheers,
Derick

Pierre Joye

12 years ago

On Mon, Apr 28, 2014 at 12:05 PM, Derick Rethans <derick@php.net> wrote:

> On Sun, 27 Apr 2014, Stas Malyshev wrote:
>
>> Hi!
>>
>> > I have a comment (not specifically to you). We can't seriously be
>> > suggesting that RMs can just revert commits. This is a really rude thing
>> > to do in an open source project. We're doing this for fun, and people
>> > immediately reverting your commits takes (more) fun out of it.
>>
>> I agree. That's why we have time limits before that should happen.
>> However, it has happened that people made commits which break things and
>> then have gone unresponsive for an extended period of time, with broken
>> commits laying there and making everybody work much harder to ensure
>> breakage does not spread (if CI is red, then any pull against it is red,
>> so we can't really trust the pulls tests, and when we merge them we
>> don't know anymore what exactly broke it and it becomes a mess very
>> quickly). So I think the normal workflow would be as follows:
>>
>> 1. Make pull
>> 2. See the pull test green
>> 3. Merge the pull
>> 4. Ensure the CI for main branch is still green
>> 5. Go have a beer/coffee/well-deserved rest
>
> Maybe, but for this to work you need to teach everybody proper git
> workflows. In a project with many infrequent committers, you're never
> going to get this done. Heck, it can be hard in a 3 man team to have
> proper git discipline.
>
>> However, if somebody commits something that breaks the CI, and is not
>> fixing it, we need to know it's OK to fix it, and we need the
>> committer to know too if he's negligent about CI hygiene his commit
>> may not be accepted.
>
> Then why do you have as an option in your voting "Revert immediately"?
> That should never be happening.

I would rather say "rarelly happen". We have seen many last minute
commits (right before RC/beta and less frequently now stable f.e.), so
yes, it should be possible. Thanks the RFC process, it is not possible
anymore to have last minute large addition like we used to have.

But that brings me back to one of my replies. To avoid an endless
commit war, we should use a restricted set of stable branches. Only
the RMs can merge/commit to the ones used for the releases. Other
developers commit to the development branches (for stable, php-next or
master), while the RMs cherry pick what we need. Doing so reduce the
issues for developers while slightly increasing the work for RMs. The
latter is not too bad as it forces them to actually do some review
before merging.

--
Pierre

@pierrejoye | http://www.libgd.org

Stas Malyshev

12 years ago

Hi!

> Then why do you have as an option in your voting "Revert immediately"?
> That should never be happening.

Because it is an option, and if we need any of the options then the
build is already broken, despite the fact it should not normally happen.
Note also "immediately" has built-in delay of 2 days. But if you really
don't like the option, don't vote for it - if it has less than 50% of
votes, it will not be accepted. However, in that case, the question
remains what to do with such commits. Having RM fix everything may not
be an option - the RM may not have time, and may not have expertise or
platform to fix a particular part. So what would we do instead?

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227