Merge lp:~adiroiban/pocket-lint/travis-ci into lp:pocket-lint

Proposed by Adi Roiban
Status: Needs review
Proposed branch: lp:~adiroiban/pocket-lint/travis-ci
Merge into: lp:pocket-lint
Diff against target: 94 lines (+40/-11) (has conflicts)
3 files modified
.travis.yml (+12/-0)
pocketlint/tests/test_json.py (+11/-5)
setup.py (+17/-6)
Text conflict in setup.py
To merge this branch: bzr merge lp:~adiroiban/pocket-lint/travis-ci
Reviewer Review Type Date Requested Status
Curtis Hovey code Needs Fixing
Review via email: mp+192779@code.launchpad.net

Description of the change

Description
===========

This branch add support for running the tests on Travis-CI.org

Changes
=======

I have created a GitHub mirror using this script https://github.com/termie/git-bzr-ng

This was requires since Travis-CI only works with GitHub.

I have updated the test.py file to return 0 on success and 1 on failure.

I have updated setup.py to install latest versions of pyflakes and pep8.

I have added travis.yml file required for enabling Travis builds

For now I have enabled Python 2.7 and 3.3.

Right now tests are executed using test.py but maybe we can make nose-runner the "official" runner.

How to test
===========

Check status at https://travis-ci.org/chevah/pocket-lint

If you want write-access to GitHub let me know your GitHub ID and I can add you to the repo.

Hope you find this useful.

For now the BZR repo is not automatically syncronized with GitHub.
Unfortunately I don't know if there is a notification API on Launchpad which can help with triggering the sync.
I can set up a cron job to keep the repos synced at 5 or 10 minutes delay.

For future branches, I plan to send them to Travis-ci before requesting a merge on LP, so this should improve the noise from review caused by python versions.

Thanks!

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

I merged your branch, made a single line decoding fix for py3.3, the committed. The tests pass, though since I do not have closure installed. 2 were skipped. I assume the tests work for you and I will verify this when I get more network.

review: Approve (code)
Revision history for this message
Curtis Hovey (sinzui) wrote :

Oops, wrong MP, sorry. I need to move this make and study it. I will claim it needs information since Lp wont let me undo this.

review: Needs Information (reviewer incompetence.)
Revision history for this message
Adi Roiban (adiroiban) wrote :

No problem.

If we have travis-ci running, I can also install google-closure in the environment and make sure google-closure tests are executed.

Revision history for this message
Curtis Hovey (sinzui) wrote :

This branch conflicts with your pep8 branch that is merged.

I would prefer that
    python_version = sys.version_info
becomes
    IS_PY3 = IS_PY3
which can be imported from formatcheck...

...but is it needed. The py2 tests fail for me with the change. py2 and py3 both pass with "enclosed in double quotes"

First differing element 0:
(2, u'Expecting property name: line 2 column 1 (char 2)')
(2, 'Expecting property name enclosed in double quotes: line 2 column 1 (char 2)')

+ [(2,
- [(2, u'Expecting property name: line 2 column 1 (char 2)')]
? ^^^^^^

+ 'Expecting property name enclosed in double quotes: line 2 column 1 (char 2)')]
? ^^ ++++++++++++++++++++++++++

review: Needs Information (code)
lp:~adiroiban/pocket-lint/travis-ci updated
510. By Adi Roiban <email address hidden>

Merge master.

511. By Adi Roiban <email address hidden>

Update code aftre review.

Revision history for this message
Adi Roiban (adiroiban) wrote :

I will use IS_PY3 ... sorry for that. I was not aware of this convention.

I have renamed requires to install_requires so that they will be installed by pip http://stackoverflow.com/questions/10335371/pip-does-not-install-dependency-declared-in-setup-requires-parameter

It looks like the PY3 check is required as this build fails : https://travis-ci.org/chevah/pocket-lint/builds/18262140

With the exception for py3 the test pass : https://travis-ci.org/chevah/pocket-lint/builds/18262337

The code works on Travis-CI https://travis-ci.org/chevah/pocket-lint

I have no idea why the MERGE markers are listed in this diff. I checked the branch and there are no markers: http://bazaar.launchpad.net/~adiroiban/pocket-lint/travis-ci/view/head:/setup.py

Please review latest changes. Thanks!

Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Adi.

This branch has a lot of merge conflicts. I think many are caused by other improvements that I just merged. I cannot make sense of them. I update setup.py and test.py since I understood your changes. Maybe you need to resubmit this branch to convince Lp to list all the conflicts.

review: Needs Fixing (code)
lp:~adiroiban/pocket-lint/travis-ci updated
512. By Adi Roiban <email address hidden>

Merge trunk.

Revision history for this message
Adi Roiban (adiroiban) wrote :

Hi and thanks for the reviews. Don't worry for the delay.

Hi, I have merged trunk.

The build passed on Travis-CI https://travis-ci.org/chevah/pocket-lint/builds/20438350

For now, travis-ci integration is not very useful for other people as I have to manually sync bzr to git.
Does LP has any hook to trigger an event when someone pushes to a branch attached to a project?

For me it is a great help since I can run test in an independent-environment... and since I use git-bzr-ng it is easy to trigger git updates and travis-ci builds.

I have not configured email notifications for Travis-CI yet so it's only my who receives email notifications.

--------

I don't know why LP shows the diff markers.

I have checked my local file at rev 512 and it looks ok.

This line should be removed

requires=['pyflakes (>=7.3)', 'pep8 (>=1.4.6)'],

and add the new format.

install_requires=['pyflakes>=0.7.3', 'pep8>=1.4.6'],

Thanks!

[1] https://github.com/termie/git-bzr-ng

Revision history for this message
Curtis Hovey (sinzui) wrote :

I don't think this will work because it is used by steup tools
    install_requires=['pyflakes>=0.7.3', 'pep8>=1.4.6'],
setup.py uses distutils which wants this format
    requires=['pyflakes (>=7.3)', 'pep8 (>=1.4.6)'],

lp:~adiroiban/pocket-lint/travis-ci updated
513. By Adi Roiban <email address hidden>

Update setup to distutils.

514. By Adi Roiban <email address hidden>

Use setuptools by default.

Revision history for this message
Adi Roiban (adiroiban) wrote :

True... I see the warning... python packaging tools are a bit of a mess :(

How 'requires' is used in the context of distutils?

I have changed to install_requires since without that, pip install will not install the dependencies.

In the penultimate revision I added both version... with duplicate info but I don't like it :(

Is there a reason to keep using distutils and not setuptools, or at least not trying to use setuptools?

In the last revision I went for setuptools as primary tools and failing to disutils if setuptools are not found.

I find setuptools install command or pip install command much useful than distutils install commnand.

-----

While checking the sdist command, I see that it runs the tests, but will not prevent building the distribution if tests fails... maybe it needs something like this:

    def run(self):
        test_loader = unittest.defaultTestLoader
        suite = unittest.TestSuite()
        for test_module in test_loader.discover('pocketlint'):
            suite.addTest(test_module)
        result = unittest.TextTestRunner(verbosity=1).run(suite)
        if len(result.failures) or len(result.errors):
            raise AssertionError('Test have failed.')

Thanks!

Revision history for this message
Adi Roiban (adiroiban) wrote :

hm... there is no penultimate commit since git to bzr conversion has squashed my git commits.

Here is the penultimate diff https://github.com/chevah/pocket-lint/commit/9928a0c606e9e965d06ccc3c6db22b1c2512e8b6

Revision history for this message
Adi Roiban (adiroiban) wrote :

Hi,

Any change you can look at the latest code?

I would be more than happy to see Travis-CI integration working and have a place to check that new changes don't break previous things.

Thanks!

Unmerged revisions

514. By Adi Roiban <email address hidden>

Use setuptools by default.

513. By Adi Roiban <email address hidden>

Update setup to distutils.

512. By Adi Roiban <email address hidden>

Merge trunk.

511. By Adi Roiban <email address hidden>

Update code aftre review.

510. By Adi Roiban <email address hidden>

Merge master.

509. By Adi Roiban <email address hidden>

Initial travis support.

508. By Curtis Hovey

Merged Adi's update to use packaged/upstream pep8.

507. By Curtis Hovey

Added simple GoChecker to hush long line and tab complaints.

506. By Curtis Hovey

Use IS_PY3

505. By Curtis Hovey

Explicit is bettern an implicit.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added file '.travis.yml'
--- .travis.yml 1970-01-01 00:00:00 +0000
+++ .travis.yml 2014-03-10 14:29:23 +0000
@@ -0,0 +1,12 @@
1language: python
2python:
3 - "2.7"
4 - "3.3"
5
6# Command to install the dependencies.
7install:
8 - "pip install -e . --use-mirrors"
9
10# Command to run the tests.
11script:
12 - python test.py
013
=== modified file 'pocketlint/tests/test_json.py'
--- pocketlint/tests/test_json.py 2014-03-09 18:52:05 +0000
+++ pocketlint/tests/test_json.py 2014-03-10 14:29:23 +0000
@@ -8,7 +8,7 @@
8 unicode_literals,8 unicode_literals,
9)9)
1010
11from pocketlint.formatcheck import JSONChecker11from pocketlint.formatcheck import IS_PY3, JSONChecker
12from pocketlint.tests import CheckerTestCase12from pocketlint.tests import CheckerTestCase
1313
1414
@@ -94,10 +94,16 @@
94 checker = JSONChecker('bogus', content, self.reporter)94 checker = JSONChecker('bogus', content, self.reporter)
95 checker.check()95 checker.check()
9696
97 self.assertEqual(97 if IS_PY3:
98 [(2, 'Expecting property name enclosed in double quotes: '98 self.assertEqual(
99 'line 2 column 1 (char 2)')],99 [(2, 'Expecting property name enclosed in double quotes: '
100 self.reporter.messages)100 'line 2 column 1 (char 2)')],
101 self.reporter.messages)
102 else:
103 self.assertEqual(
104 [(2, 'Expecting property name: line 2 column 1 (char 2)')],
105 self.reporter.messages)
106
101 self.assertEqual(1, self.reporter.call_count)107 self.assertEqual(1, self.reporter.call_count)
102108
103 def test_compile_error_on_multiple_line(self):109 def test_compile_error_on_multiple_line(self):
104110
=== modified file 'setup.py'
--- setup.py 2014-03-10 01:13:06 +0000
+++ setup.py 2014-03-10 14:29:23 +0000
@@ -6,14 +6,21 @@
6)6)
77
8import subprocess8import subprocess
9
10from distutils.core import (
11 Command,
12 setup,
13)
14from distutils.command.sdist import sdist
15import unittest9import unittest
1610
11try:
12 from setuptools import (
13 Command,
14 setup,
15 )
16 from setuptools.command.sdist import sdist
17except ImportError:
18 from distutils.core import (
19 Command,
20 setup
21 )
22 from distutils.command.sdist import sdist
23
1724
18class SignedSDistCommand(sdist):25class SignedSDistCommand(sdist):
19 """Sign the source archive with a detached signature."""26 """Sign the source archive with a detached signature."""
@@ -64,7 +71,11 @@
64 package_data={71 package_data={
65 'pocketlint': ['jsreporter.js'],72 'pocketlint': ['jsreporter.js'],
66 'pocketlint/contrib': ['fulljslint.js']},73 'pocketlint/contrib': ['fulljslint.js']},
74<<<<<<< TREE
67 requires=['pyflakes (>=7.3)', 'pep8 (>=1.4.6)'],75 requires=['pyflakes (>=7.3)', 'pep8 (>=1.4.6)'],
76=======
77 install_requires=['pyflakes>=0.7.3', 'pep8>=1.4.6'],
78>>>>>>> MERGE-SOURCE
68 scripts=['scripts/pocketlint'],79 scripts=['scripts/pocketlint'],
69 cmdclass={80 cmdclass={
70 'check': Check,81 'check': Check,

Subscribers

People subscribed via source and target branches

to all changes: