[Twisted-Python] Accepting/merging patches for unsupported platforms without CI

[Glyph]

> On Nov 26, 2017, at 10:23 AM, Adi Roiban <adi at roiban.ro> wrote:
> 
> Hi,
> 
> What is the procedure for accepting patches for platforms for which we
> don't have a continuous testing system ?
> 
> This is a follow up of the review from
> https://twistedmatrix.com/trac/ticket/9323
> 
> There is a patch for Cygwin but AFAIK there is no current builder for
> running Twisted with Python on Cygwin.
> 
> My first reaction was that before accepting such patches we should set
> up a continuous testing system.
> 
> With such a system in place we can run the tests to see that the
> reported bug is present in trunk and that it is fixed in a branch.
> 
> If that can't be done, we need at least someone to manually run the
> tests to check the patch.
> 
> Twisted is supported on so many systems and I don't think that is OK
> to ask reviewers to have each supported system at hand, ready to fire
> a manual test.
> 
> Any comments?
> 

To support a platform—i.e. to promise we will not break it in the future—we  have to have a builder capable of running Twisted on that platform.

To accept a patch though, the important thing is to have test coverage. If the change in question is a platform-specific feature that there’s no way to test in a different environment, we would indeed need to block on the availability of a builder.

Quite often—as I believe is the case for the patch you’re referring to here—patches are adjusting behaviors which would be difficult to cleanly integration-test on the platform in question anyway, and the appropriate thing to do is to make some mock match the observed behavior of the platform in question.  In this case it would be nice if the reviewer could verify this, but if the patch submitter’s description of observed behavior is clear, and their test mock implements it correctly, I would say that we can give them the benefit of the doubt and accept such patches even if the reviewer doesn’t have such a platform available.  Assuming all the changed lines are covered adequately for our supported platforms’ behavior as well, of course, and we aren’t risking breaking them.

How do you feel about this answer?

-g