Opened 6 years ago

Closed 4 years ago

#7229 enhancement closed fixed (fixed)

Add Travis CI configration file

Reported by: Adi Roiban Owned by: Ying Li <cyli@…>
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: 7229-rodrigc-travis
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

Add a Travis CI configuration file so developers can enable Travis CI on their Twisted forks on GitHub

Attachments (1)

7229-1.diff (6.1 KB) - added by Adi Roiban 6 years ago.

Download all attachments as: .zip

Change History (45)

comment:1 Changed 6 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Here is my initial travis-ci configuration.

While Twisted buildbot configuration is public I fount it much easier to do reverse engineering and trial and error.

For example in all that braid/fabric wizardry I could not find what version of pep8 is required for twistedchecker step.


If everthing is OK I will write a similar tox file (or setup some tools to do the conversion)

Before trying Travis CI I tried to use tox, but things got complicated since developers would need to install python 2.6, 2.7 , 3.3 on their local computer + virtualenv and other packages.

It is easier to start with Travis CI since it does not need instruction for setting up the local system.

The tox config should reuse the same run-test-suite script.


Once this branch is merged, Travis CI does not need to be enabled for main Twisted git mirror, but users having Twisted forks on GitHub can enable Travis for their own repos.

This is how I am using Travis to work on this patch. Travis-Ci in only enabled on Chevah fork for Twisted

https://travis-ci.org/chevah/twisted


pyflakes test are failing but I hope that soon all pyflakes errors will be fixed.


gi and gtk3 reactor tests are also failing... I am not sure why... also not sure if I should care about gtk2, gi and gtk3 reactors as there were not in the list of supported builders in Buildbot.


I have no idea why on Travis CI worker when deleting a monitored folder the event is IN_ATTRIB rather than IN_DELETE_SELF. This is why I have deleted tests that rely on IN_DELETE_SELF.


Twistedchecker is disabled since diff functionality is broken. I will try to fix that and then enable twistedchecker.


I have attached a patch based on trunk.

Thanks!

Changed 6 years ago by Adi Roiban

Attachment: 7229-1.diff added

comment:2 Changed 6 years ago by hawkowl

Owner: set to hawkowl

comment:3 Changed 6 years ago by hawkowl

(In [43348]) apply patch from adiroiban. refs #7229

comment:4 Changed 6 years ago by hawkowl

(In [43349]) apply patch from adiroiban. refs #7229

comment:5 Changed 6 years ago by hawkowl

Keywords: review removed

This patch makes me super excited, as it helps with lowering the bar for providing patches that work well on a significant chunk of our buildbots, without actually having to be run on the buildbots.

I have tested it on a fork, and it works well for getting the base up and running easily. Even though some (rarer) builds fail, we can always fix that later.

+1, will merge.

comment:6 Changed 6 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [43353]) Merging travis-ci-7229: Add a Travis CI configuration

Author: adiroiban Reviewer: hawkowl Fixes: #7229

comment:7 Changed 6 years ago by Jean-Paul Calderone

In general, it is incorrect to set the skip attribute of a test method to None. Consider:

exarkun@top:~$ cat > /tmp/test_test.py
from twisted.trial.unittest import TestCase

class SomeTest(TestCase):
	skip = "yes"

	def test_foo(self):
		self.fail("bar")
	test_foo.skip = None

exarkun@top:~$ trial /tmp/test_test.py 
test_test
  SomeTest
    test_foo ...                                                         [FAIL]

===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/tmp/test_test.py", line 7, in test_foo
    self.fail("bar")
twisted.trial.unittest.FailTest: bar

test_test.SomeTest.test_foo
-------------------------------------------------------------------------------
Ran 1 tests in 0.102s

FAILED (failures=1)
exarkun@top:~$ 

The general nature of this change is also a little disconcerting. It does seem nice to help make testing easier for non-committers but since committers don't pay any attention to the status of the test suite on travis, there's no reason to think the test suite will continue to pass there as maintenance continues.

comment:8 Changed 6 years ago by Jonathan Lange

This change appears to break the build on some of our builders.

[ERROR]
Traceback (most recent call last):
  File "/Users/twisted/buildslave/osx10.6-py2.6-select/Twisted/twisted/internet/test/test_inotify.py", line 37, in setUp
    self.inotify = inotify.INotify()
exceptions.AttributeError: 'NoneType' object has no attribute 'INotify'

twisted.internet.test.test_inotify.TestINotify.test_deleteSelf
twisted.internet.test.test_inotify.TestINotify.test_ignoreFilePath
twisted.internet.test.test_inotify.TestINotify.test_seriesOfWatchAndIgnore
twisted.internet.test.test_inotify.TestINotify.test_simpleDeleteDirectory

Also on some Windows builders.

The issue seems to be that the new skip for these tests overrides the class-level skip.

I don't quite have time to roll back just now.

comment:9 Changed 6 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

(In [43369]) Revert r43353 - test suite regression on Windows

Reopens: #7229

=============================================================================== [ERROR] Traceback (most recent call last):

File "C:\Users\Build Slave\bot-glyph-6\windows7-64-py2.7-select\Twisted\twisted\internet\test\test_inotify.py", line 37, in setUp

self.inotify = inotify.INotify()

exceptions.AttributeError: 'NoneType' object has no attribute 'INotify'

twisted.internet.test.test_inotify.TestINotify.test_deleteSelf twisted.internet.test.test_inotify.TestINotify.test_ignoreFilePath twisted.internet.test.test_inotify.TestINotify.test_seriesOfWatchAndIgnore twisted.internet.test.test_inotify.TestINotify.test_simpleDeleteDirectory


comment:10 Changed 6 years ago by Jean-Paul Calderone

I talked to hawkowl about how this slipped through. The general idea was that travis is a thing kind of like buildbot. hawkowl looked at travis and saw the tests were passing and mistook that for meaning that the change didn't introduce any failures. The mistake there, of course, was that travis doesn't run the exact same tests as buildbot (one particular difference being that there is no Windows coverage on travis at all). The problem was in the area that buildbot tests and travis doesn't.

We might want to add a note to the travis config here to emphasize that buildbot is the canonical source of working/failing information. Committers shouldn't be tricked into a false sense of correctness by the travis results. (Thus if there is future maintenance done on the travis config, the committer who reviews the change might hopefully receive this reminder. I dunno, this is the best idea I've had on this so far. Or maybe just the most feasible: where is the trac "merge" button that is gated on good buildbot results?)

comment:11 Changed 6 years ago by Adi Roiban

hawkowl do you plan to continue working on this?

Now that I can run buildbot test, I can try to continue working on this so that buildbot is happy with the patch and also investigate why inotify tests are failing on Travis-CI

I think that having an "official" travis-ci file should help people forking on GitHub as they can enable Travis-Ci for their on forks.

comment:12 Changed 6 years ago by Adi Roiban

Travic-CI should NOT replace buildbot.

Also the current patch works on Travis by disabling some tests. I sent the patch for review just to get feedback and help from other developers.

My purpose was to provide a tool to allow a non-comitter to run some tests in an isolated environment.

Now I think that this ticket should be closed as won't fix and rather than using Travis-CI we should work at improving the official Buildbot server to allow non-commiters to tests their changes.

#7228 will help in this but tests are still executed on the same system used for development ... but official tests require Linux x86, Linux x64, Windows XP, Windows 7, OSX.

Even with #7228 merge a non-comitter will need to setup at least 5 systems to tests their patches... and I think that this is a huge requirement.

Also twistedchecker/pyflakes/api-docs tests depende on Buildbot and can not be easily replicated on local system or Travis-CI

comment:13 Changed 4 years ago by Craig Rodrigues

Keywords: review added

I went with a more minimal .travis.yml file, which invokes the test environments specified in tox.ini.

Please review:

https://github.com/twisted/twisted/pull/64

comment:14 Changed 4 years ago by Adi Roiban

It looks good.

I think that we should re-use tox and start with simple environments, just like you did. Thanks.


  1. I hope that soon we can have a pyflakes test and the new great thing from Amber which checks for missing news file fragments :)

Will it be easy to add pyflakes tox to the current configuration?

I don't know how tox-travis works... so my question is.

Do we really need tox-travis? Why not manually call the tox targets?


  1. Can we also have some documentation for this... so that old and new contributors will know what are these Travis builds?

But maybe this should be documented after this lands and we enable the PR hooks for Travis-CI.


  1. There is one thing that maybe we can fix in the current branch:
This log is too long to be displayed. Please reduce the verbosity of your build or download the raw log.

I saw that you used a hack... but can we have the log less verbose in Travis ?

We can continue to have huge logs in buildbot.

If you live with a fiber connection in your move, big logs should not be a problem... but for the buildbot huge logs were always a problem as I use either very slow ADSL or 3G connections.

Also, for new contributions, I think that all these huge logs can be a noise as they should only care about the test which fails.

I am not sure if anybody is constantly checking the full log to detect which tests are skipped.

comment:15 Changed 4 years ago by Craig Rodrigues

Using tox-travis makes things easier. If you don't use it, then you need to create a lot more environments in the tox.ini, and it gets quite messy looking.

tox-travis is in pypi: https://pypi.python.org/pypi/tox-travis and if you look at the actual code, it is super small and concise, yet is worth it, due to the convenience of not cluttering up tox.ini with environments.

comment:16 Changed 4 years ago by Craig Rodrigues

Yes, adding pyflakes or other things is doable.

comment:17 Changed 4 years ago by Craig Rodrigues

In terms of the logs, in Travis, if the entire log is greater than 4 MB, the build is terminated and marked as failed, regardless of if the build actually succeeded or not. That is why I redirected the log output to a file, and then printed the last 2 MB of the log file on completion of the tests.

comment:18 Changed 4 years ago by Adi Roiban

Keywords: review removed
Owner: changed from hawkowl to Craig Rodrigues
Status: reopenednew

Great. tox-travis looks good for running a pyflakes or topfiles checker.

Now here is the review.

  1. Do we need sudo for these tests?

  1. Instead of the log hack, can we just run the tests in Travis in non-verbose mode ... for example with --reporter text this will report the skipped tests.

maybe we can have something like this in tox.ini

commands =
    {tests,nomodules}: {envbindir}/trial --reactor={env:TWISTED_REACTOR:default} --reporter={env:TRIAL_REPORTER:verbose}  {posargs:twisted}

in .travis.yml we can have

env:
  global:
    - TRIAL_REPORTER=text

  1. The tests without modules is a helpful suite to detect problem in the skip conditions.... but maybe we can have them introduced in a separate ticket.

It can only run on py27


Now I need to find out how the documentation for working with GitHub branches in Twisted.

For me, 2. is a blocker ... 1 and 3 are minor comments.

Please review my comments and resubmit.

Thanks!

comment:19 Changed 4 years ago by Craig Rodrigues

sudo is not needed to run the tests. If you look at the test output here: https://travis-ci.org/rodrigc/twisted/builds/132002003

you will see:

This job is running on container-based infrastructure, which does not allow use of 'sudo', setuid and setguid executables.

If you require sudo, add 'sudo: required' to your .travis.yml

See https://docs.travis-ci.com/user/workers/container-based-infrastructure/ for details.

comment:20 Changed 4 years ago by Craig Rodrigues

Are you sure that nomodules tests only run on py27? If I look at the full raw log on the py33 build:

https://travis-ci.org/rodrigc/twisted/jobs/132002005

it looked like they ran. Look for: py33-nomodules-posix runtests:

comment:21 Changed 4 years ago by Adi Roiban

Here is the list of "official" builders for Twisted https://buildbot.twistedmatrix.com/boxes-supported?branch=trunk&num_builds=1

nomodules only runs on py2.7

I don't see much value in also having nomodules for py3


sorry about the sudo stuff... I have not used travis-ci for quite some time.

comment:22 Changed 4 years ago by Craig Rodrigues

Keywords: review added
Owner: changed from Craig Rodrigues to Adi Roiban

I modified the patch with your suggestions: ​https://github.com/twisted/twisted/pull/64

The build and logs are here: https://travis-ci.org/rodrigc/twisted/builds/132114075

The log output came out to about 1.7mb per build. This was under Travis's 4mb log threshold, so Travis did not terminate the build.

Looking at the build logs, py27-tests-posix and py27-nomodules-posix print short output, but py27-coverage-posix printed full output. Any ideas?

comment:23 Changed 4 years ago by Adi Roiban

Ok... a bit better... but the log is still huge.

Now I feel that tox-travis is setting up the wrong Travis matrix.

instead of having a single travis build for all py2.7 builders, I was expecting that each py2.7 tests suite will have its own build.

For example py27-nomodules-posix will have its own build.

With the current huge log I find it hard to see the results from py27-nomodules-posix


You should also update the coverage runner to be

coverage: coverage run -p --rcfile={toxinidir}/.coveragerc {envbindir}/trial --reactor={env:TWISTED_REACTOR:default} --reporter={env:TRIAL_REPORTER:verbose} {posargs:twisted}

in this way, the coverage tests will also be reported in less verbose mode


I see that some tests are skipped since pydoctor and sphinx is not installed... I think that we should install them... but maybe this is a separate ticket

comment:24 Changed 4 years ago by Adi Roiban

Maybe my request about 1 test suite mapped to 1 build matrix type is already reported here https://github.com/ryanhiebert/tox-travis/issues/4

comment:25 Changed 4 years ago by Craig Rodrigues

I incorporated your change:

https://github.com/twisted/twisted/pull/64

and it reduced the output of the coverage tests: https://travis-ci.org/rodrigc/twisted/builds/132147439

Please review

comment:26 Changed 4 years ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Craig Rodrigues

Much, much better :)

  1. Do we need to run both py27-tests-posix and py27-coverage-posix ?

Maybe py27-coverage-posix is enough.

Same for py33-tests-posix and py33-coverage-posix and for 3.4 and 3.5

  1. I don't think that py33-nomodules-posix, py34-nomodules-posix and py35-nomodules-posix add any new coverage so we can just skip them.

If you think that we need nomodules tests for each python version, than we should also update the builder on buildbot to include these type of builders.


I hope that less tests should reduce the log size and the load on Travis-CI

I feel that the current configuration is abusing the free Travis-CI service by running extra tests which are not really required.

Please check my comments.

Many thanks!

comment:27 Changed 4 years ago by Craig Rodrigues

Keywords: review added
Owner: changed from Craig Rodrigues to Adi Roiban

I modified the patch with your suggestions: ​​https://github.com/twisted/twisted/pull/64

  • on 2.7 build py27-coverage-posix,py27-nomodules-posix
  • on 3.3 build py33-coverage-posix
  • on 3.4 build py34-coverage-posix
  • on 3.5 build py35-coverage-posix

The build and logs are here: https://travis-ci.org/rodrigc/twisted/builds/132670177

Please review.

Last edited 4 years ago by Craig Rodrigues (previous) (diff)

comment:28 Changed 4 years ago by Adi Roiban

Branch: 7229-rodrigc-travis
Keywords: review removed
Owner: changed from Adi Roiban to Craig Rodrigues

It looks good and I think that this is a good start.

I have added a topfile and the badge.

I have enabled Travis for twisted/twisted

I have removed the comment explaining how tox-travis works and replaced it with just a link to tox-travis. In the future tox-travis might evolve and the comment might get out of date.

Also, for travis-ci I think that we can call pip install a single time to install both tox and tox-travis

Waiting for builds to get green and some more feedback from other devs and will merge.

Many thanks for this great branch!

comment:29 Changed 4 years ago by Adi Roiban

looking forward for the follow up branch and which we could add the topfile checker and pyflakes check in travis-ci :)

comment:30 Changed 4 years ago by Adi Roiban

and then maybe we should also add a link to travis, from the Trac ticket

comment:31 Changed 4 years ago by Adi Roiban

.... and we might want to run Travis only for PR and master... not sure if we want travis for every commit in every branch as we have buildbot

comment:32 Changed 4 years ago by Craig Rodrigues

If https://travis-ci.org/twisted/twisted was only configured to build pull requests, I would be fine with that. The buildbots could certainly be used for the branches and trunk.

comment:33 Changed 4 years ago by Adi Roiban

buildbot looks good... but travis is failing.

It looks like a concurrency issue https://travis-ci.org/twisted/twisted/builds/132695019

I have no idea why it was passing on rodrigc fork


I think that we can start with builds for pull request and for trunk.. so that we have a badge :) ... but I hope that the badge will always be green.

I don't see the point of a build status badger for "trunk" as that branch should always be green, but I think that the badge is a sign for new contributors that we care about testing... and they should too :)

Here is the PR for the branch hosted in main Twisted repo https://github.com/twisted/twisted/pull/73

comment:34 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Craig Rodrigues deleted

Ok.

So we have https://github.com/twisted/twisted/pull/73 in which the integration is tested.

I have updated the documentation to talk about Travis CI.

Please check that all is ok.

I think that Amber or Glyph should also take a look, just to make sure all is ok.

Here is this ticket and Travis CI configuration back for review.

comment:35 Changed 4 years ago by Craig Rodrigues

Owner: set to Adi Roiban

Please add https://github.com/twisted/twisted/pull/87 to your branch before handing to amber or glyph.

The latest topfile commit broke running Travis on PR's, like this one: https://travis-ci.org/twisted/twisted/builds/133122284

comment:36 Changed 4 years ago by Adi Roiban

Thanks!

For now I will include the fix in travis config file.

I would say that the topfile tox rule should be updated so that it will fetch trunk before run... but this should be done in a separate ticket see #8373

Hope all is ok now.

Thanks again rodrigc :)

comment:37 Changed 4 years ago by Craig Rodrigues

I thought about modifying the tox.ini or the script which invokes git diff --name-only origin/trunk..., but thought that might face more opposition than modifying the travis.yml file, because for now, this problem is only seen in travis.

If you do: git clone https://github.com/twisted/twisted

you don't see the problem, because that clones the whole repository and all the refs (branches and tags) including trunk

If you do the following (like what Travis does):

git clone --depth 50 --branch=7229-rodrigc-travis https://github.com/twisted/twisted

That clones only a subset of the repository, with only the 7229-rodrigc-travis branch, and without the trunk branch.

Since most folks are doing git clone the "normal" way, I thought to put the workaround only in the Travis file.

Thanks again for the review.

comment:38 Changed 4 years ago by Adi Roiban

AFAIK buildot is not even fetching the branch, but goes directly for the commit with a depth of 1 ... so I assume this will also be a problem on buildbot.

some feedback over IRC

(18:25:56) rodrigc: only thing I would say is, if you can either remove restriction for travis-ci to only build on trunk
(18:26:06) rodrigc: that would be nice
(18:26:34) adiroiban: does it make sense to run the tests for any branch without a PR ?
(18:26:42) adiroiban: I don't want to abuse the free travis-ci service
(18:27:09) rodrigc: well what I found is that builds on PR don't work with that addition
(18:27:27) rodrigc: in my fork
(18:28:35) rodrigc: also it is useful for someone working in branches on their own fork to be able to test with travis
(18:29:17) rodrigc: but for now, the only thing I care about is for the main twisted/twisted repo to have travis build PR's.  If it can do that, that is good enough
(18:29:38) rodrigc: but that is hard to see if it works until .travis.yml gets merged to trunk

I still don't see how enabling Travis-CI for any branch will help. People can just run all those tests on their local system, then send the PR ... wait for travis-ci results and push another commit in that PR .. and when PR is green, ask for a review :)

But we need to document the new Github PR workflow and contribution steps

comment:39 Changed 4 years ago by Adi Roiban

Owner: Adi Roiban deleted

Waiting for someone else to review the branch as I have also made some changes to it.

comment:40 Changed 4 years ago by Daniel Mizyrycki

Looks good to me. I checked the code and confirm Travis-CI is properly running. Reassigning to @adiroiban

comment:41 Changed 4 years ago by Daniel Mizyrycki

Author: adiroiban
Keywords: review removed

comment:42 Changed 4 years ago by Adi Roiban

Keywords: review added

I would like to get a review from at least another Twisted committer to check that all is ok and this has no negative side effects on the Twisted development process.

Thanks!

comment:43 Changed 4 years ago by Ying Li

Keywords: review removed

Thank you for all your hard work on this adiroiban and rodrigc! This greatly improves feedback to contributors. I've clarified some of the grammar on the CONTRIBUTING doc, but other than that this looks good to me. Will merge as soon as CI passes.

comment:44 Changed 4 years ago by Ying Li <cyli@…>

Owner: set to Ying Li <cyli@…>
Resolution: fixed
Status: newclosed

In e56d76f:

Merge pull request #73 from twisted/7229-rodrigc-travis: add travis configuration to cover some buildbot tests

Author: adiroiban, rodrigc
Reviewer: hawkowl, adiroiban, mzdaniel, cyli
Fixes: #7229

Sets up a travis configuration to cover linux py27, py33, py34, py35 tests, and updates some of the Github contribution documentation to reference the travis results.

Note: See TracTickets for help on using tickets.