ship-it: setup a travis (or equivalent)

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sylvestre, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
It would be nice if flake8 and the unitary tests could be launched after each checkin in ship-it.
We should probably move it to github and use Travis. That'll require coordination with IT.
(Reporter)

Updated

4 years ago
Depends on: 1112093
Created attachment 8555911 [details] [diff] [review]
enable_travis.diff

To have files in place and be ready for travis.
It also adds tox.ini to simplify isolated tests.
Attachment #8555911 - Flags: review?(bhearsum)
(Reporter)

Comment 3

4 years ago
\o/
If you can figure out how to run the js unit tests, that would be great too :)
They are in kickoff/test/js/
(In reply to Sylvestre Ledru [:sylvestre] from comment #3)
> \o/
> If you can figure out how to run the js unit tests, that would be great too
> :)
> They are in kickoff/test/js/

The following scenario worked for me:

$ cat grunt.js
module.exports = function(grunt) {
    grunt.initConfig({
        qunit: {
            files: ['kickoff/test/js/jstest.html']
        }
    });
    grunt.loadNpmTasks('grunt-contrib-qunit');
    grunt.registerTask('test', 'qunit');
};

$ ln -s =nodejs ~/bin/node  # to make some packages happy
$ npm install grunt-cli grunt-contrib-qunit &&  nodejs ./node_modules/grunt-cli/bin/grunt --gruntfile grunt.js --verbose qunit
Comment on attachment 8555911 [details] [diff] [review]
enable_travis.diff

Review of attachment 8555911 [details] [diff] [review]:
-----------------------------------------------------------------

::: .travis.yml
@@ +1,3 @@
> +language: python
> +python:
> +  - "2.7"

We run 2.6 on the server, we should test that. We can test 2.7 as well if you want.

::: tox.ini
@@ +10,5 @@
> +    -rrequirements/compiled.txt
> +    coverage
> +    nose
> +    rednose
> +    flake8

You should put these other deps into dev.txt and include that instead IMO.
Attachment #8555911 - Flags: review?(bhearsum) → review-
Created attachment 8556078 [details] [diff] [review]
enable_travis.diff

Bah, I forgot that we use python 2.6 in production. Good catch!

I added support for 2.6 in tox.ini (it was easy) and added suport for JS unit tests.
Attachment #8555911 - Attachment is obsolete: true
Attachment #8556078 - Flags: review?(bhearsum)
Comment on attachment 8556078 [details] [diff] [review]
enable_travis.diff

302 peterbe!
Attachment #8556078 - Flags: review?(bhearsum) → review?(peterbe)
Comment on attachment 8556078 [details] [diff] [review]
enable_travis.diff

Review of attachment 8556078 [details] [diff] [review]:
-----------------------------------------------------------------

First of all,
the best place to test all of this is to make a GitHub PR and let Travis build that and see if it works as expected. 
Why you're using Bugzilla and not GitHub PR mind boggling :) but independent of that I really think you should make a PR which gets built on Travis. You can still do the r? stuff here but it would be best to see it in real action.

Secondly, 
I'm not familiar with the project but although doing tox on your laptop is great (great because it's almost identical to how travis does it) it's still very slow. 
A real developer doesn't want to run the whole suite every time. Especially not in multiple python versions. And definitely doesn't have the patience to be slowed down by coverage. 
So, I would flesh out the README to focus on running nosetests. And mention that if you want to run the whole suite like Travis does you can run `pip install tox; tox`.
If you want to get productive you need to become familiar with running nosetests and how to run small subsets of tests etc. 

I can't find anything obvious wrong with the patch but my r+ is ONLY IF YOU RUN THIS ON GITHUB WITH TRAVIS RUNNING THE PULL REQUEST :) haha

::: README
@@ +37,5 @@
>  		- Switching to a 32-bit vagrant box seems to be a temporary fix
>  
>  
>  To run Python tests:
> +* install python-tox

How about changing this to `pip install tox`?

@@ +43,5 @@
>  
>  To run JS tests:
>  * firefox kickoff/test/js/jstest.html
> +or using node.js
> +* npm install grunt-cli grunt-contrib-qunit &&  node ./node_modules/grunt-cli/bin/grunt --gruntfile grunt.js --verbose qunit

This is a bit odd. You normally have a package.json file and then if you just run `npm install` in the same directory it automatically notices the `devDependencies` key in that package.json file and it even makes them executable. 

Start with:
{
  "name": "nameofyourproject",
  "title": "Verbose name of your Project",
  "version": "0.0.1",
  "author": {
    "name": "Rel Eng",
    "company": "Mozilla"
  },
  "devDependencies": {}
}

Then, to fill in the `devDependencies` you run, on your local laptop::

  npm install grunt-cli grunt-contrib-qunit --save-dev

That should set up the right versions in the package.json.
Then add that package.json to the git repo (and include in the patch).

Now the instructions can simply become::

 or using node.js
 npm install
 grunt --verbose qunit

::: grunt.js
@@ +5,5 @@
> +        }
> +    });
> +    grunt.loadNpmTasks('grunt-contrib-qunit');
> +    grunt.registerTask('test', 'qunit');
> +};

This file should be called `Gruntfile.js` not `grunt.js`. At least, that's why I always see. That way you can just run `grunt qunit` once grunt is installed and on the PATH. 

Also, don't you need to have a default for when someone just types `grunt` without a second command. E.g.::

  grunt.registerTask('default', ['test']);

::: tox.ini
@@ +17,5 @@
> +[testenv:py27-coveralls]
> +deps=
> +    python-coveralls
> +commands=
> +    coveralls -i

Hmm... Not so sure about putting coveralls in the tox.ini.
The tox.ini is a thing that developers use on their laptop. They shouldn't ever run coveralls. Coveralls is best run on travis. Only. 

I would propose that you put::

  after_success:
      # Report coverage results to coveralls.io
      - pip install coveralls
      - coveralls

in your .travis.yml file instead of having it here in tox. However, I don't know how you get it to just upload the coverage file for the 2.7 run.
Attachment #8556078 - Flags: review?(peterbe) → review+
(In reply to Ben Hearsum [:bhearsum] from comment #1)
> We should probably move it to github and use Travis. That'll require
> coordination with IT.

+1

I'm not sure what this project even is but yes to github+travis. 
Out of curiosity, why does this require coordination with IT?
(In reply to Peter Bengtsson [:peterbe] from comment #9)
> (In reply to Ben Hearsum [:bhearsum] from comment #1)
> > We should probably move it to github and use Travis. That'll require
> > coordination with IT.
> 
> +1
> 
> I'm not sure what this project even is but yes to github+travis. 
> Out of curiosity, why does this require coordination with IT?

We use Chief to deploy ship it changes, and it's hooked up to the git.m.o repository. We need to make sure it doesn't break when we move the repo.
Comment on attachment 8556078 [details] [diff] [review]
enable_travis.diff

Peter, thanks a lot for you great review. I addressed most of the issues you mentioned in your comment and pushed a slightly modified version:

https://git.mozilla.org/?p=build/release-kickoff.git;a=commitdiff;h=37814db0d8f1ad4fdd727fac66fc44e57c2bd529
Attachment #8556078 - Flags: checked-in+
So, has it worked on Travis yet?
(Reporter)

Comment 13

4 years ago
I tried a pull request but nothing happened:
https://github.com/mozilla/ship-it/pull/1
(Reporter)

Comment 14

4 years ago
That is now working!
https://travis-ci.org/mozilla/ship-it
and coverage:
https://coveralls.io/r/mozilla/ship-it
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.