Simple install tests on Travis CI #23

hugovk · 2018-04-17T10:10:58Z

Something to get started with for #18.

Example output: https://travis-ci.org/hugovk/get-pip/builds/367577764

webknjaz · 2018-04-18T12:03:47Z

.travis.yml

+language: python
+
+python:
+ - "3.7-dev"


You don't really need quotes or tab here.

Not a problem with 3.7-dev, nor with any of the current Python versions (will there be a Python 3.10?), but it's good practice because:

In YAML, - 7.10 would evaluate to version 7.1, and not 7.10. ...

In YAML, 7.8 is a float, "7.8" is a string. Software version numbers should be strings, not floats.

travis-ci/docs-travis-ci-com#1537

But happy to remove them, let me know.

Well, it's just my personal preference, because it looks cleaner. And I use it this way everywhere, so I can tell you that it doesn't cause problems in my experience.

webknjaz · 2018-04-18T12:04:38Z

curl-install.sh

+#!/usr/bin/env bash
+
+# Exit on error, run verbose
+set -ev


how about adding -uo pipefail?

webknjaz · 2018-04-18T12:05:52Z

local-install.sh

+# Run from a script to make sure Travis CI handles failures properly:
+# http://steven.casagrande.io/articles/travis-ci-and-if-statements/
+
+if   [ $TRAVIS_PYTHON_VERSION == 2.6 ]; then python 2.6/get-pip.py;


I think, these two scripts could be turned into one: they are 90% copy-paste with just one variable thing.

you can have a function which either cats script or wgets it

@hugovk

Smth like helpers.sh:

function cat_get_pip() { local version="$1" local path="get-pip.py" [[ -n "$version" ]] && path="$version/$path" local src="$2" local cmd=cat if [[ "$src" == "remote" ]] then path="https://bootstrap.pypa.io/$path" cmd='wget -O - ' elif [[ "$src" != "local" ]] >&2 echo Wrong source argument: $src exit 1 fi $cmd $path }

and then have:

before_script: - source helpers.sh script: - cat_get_pip "$TRAVIS_PYTHON_VERSION" "$SCRIPT_SRC" | python - matrix: global: - SCRIPT_SRC=local - SCRIPT_SRC=remote

webknjaz · 2018-04-18T12:07:04Z

curl-install.sh

+# Run from a script to make sure Travis CI handles failures properly:
+# http://steven.casagrande.io/articles/travis-ci-and-if-statements/
+
+if   [ $TRAVIS_PYTHON_VERSION == 2.6 ]; then curl https://bootstrap.pypa.io/2.6/get-pip.py | python;


shell scripts don't require semi-colons in the the end of lines: \n is already a separator itself.

webknjaz · 2018-04-18T12:07:48Z

curl-install.sh

@@ -0,0 +1,15 @@
+#!/usr/bin/env bash


Nipick: I'd prefer having space after #! for the sake of readability.

webknjaz · 2018-04-18T12:08:29Z

.travis.yml

+
+script:
+ # Simple test runs
+ - ./local-install.sh


local/remote could be put into env var and then reused in matrix.

I'd prefer not to mix these things in a single job. Having them separate helps understand what fails better and faster.

webknjaz · 2018-04-18T12:09:47Z

.travis.yml

+ - ./curl-install.sh
+
+after_script:
+ # Static analysis: for info, won't fail the build


Well, maybe it should fail the build.

there's just a few linting errors, so why don't we get rid of them and make sure they won't happen to be repeated?

webknjaz

This is a good effort @hugovk, I like it!

Another thing, I'd like to add (maybe in subsequent PR) is automating deployment (how does it get to bootstrap.pypa.io? this could be even hosted on GitHub Pages) and automating bundling of pip zip blob to the end of the scripts.

webknjaz · 2018-04-18T12:34:19Z

.travis.yml

 - pip install pyflakes pycodestyle
- - pyflakes .
- - pycodestyle --statistics --count .
+ - flake8 .


You known that flake8 uses pyflakes with a bit permissive config, right?
Oh, and how about using black?
Also idea: there's a nice project pre-commit.com, which enables users to have local hook in git as well as repeat that in CI. It supports running all linters above (and more) and I can help configuring it if needed.

I switched to flake8 because pyflakes doesn't respect the # noqa comments. But now flake8 doesn't install for all Python versions. But we don't need to run it on all, just a recent one. Will update.

Black could be good too, but I'd probably wait until it's at least in beta, as things are still changing there.

we can add extra allowed to fail job for black

webknjaz · 2018-04-18T12:52:01Z

@hugovk it looks like this got folder, so copying it here:

Smth like helpers.sh:

function cat_get_pip() {
  local version="$1"
  local path="get-pip.py"
  [[ -n "$version" ]] && path="$version/$path"
  local src="$2"
  local cmd=cat
  if [[  "$src" == "remote" ]]
  then
    path="https://bootstrap.pypa.io/$path"
    cmd='wget -O - '
  elif [[ "$src" != "local" ]]
    >&2 echo Wrong source argument: $src
    exit 1
  fi
  
  $cmd $path
}

and then have:

before_script:
- source helpers.sh
script:
- cat_get_pip "$TRAVIS_PYTHON_VERSION" "$SCRIPT_SRC" | python -

matrix:
  global:
  - SCRIPT_SRC=local
  - SCRIPT_SRC=remote

webknjaz · 2018-04-18T13:19:10Z

.travis.yml

+- source helpers.sh
+
+install:
+- if [ $TRAVIS_PYTHON_VERSION == 3.6 ]; then pip install flake8; fi


Please use matrix.include or jobs.include instead.

webknjaz · 2018-04-18T13:58:57Z

@hugovk I've sent you a few patches via hugovk#1

webknjaz · 2018-04-18T15:16:27Z

@hugovk ^^ I've fixed a few bugs in my PR above, it's now ready to be merged-in to your branch.

webknjaz · 2018-04-18T16:12:53Z

As for real tests (future PR to not block this initial one), it's possible to use bats test framework https://github.com/sstephenson/bats

hugovk · 2018-04-18T17:12:36Z

hugovk#1 is merged into this, thanks!

webknjaz · 2018-04-19T12:30:57Z

Hello @dstufft,

This requires your attention. I suggest that you merge this right away and then we'd have some follow-up PRs improving things. (unless there's smth critical you want to fix in this one).

pradyunsg · 2018-04-20T07:21:49Z

requirements.txt

@@ -1,2 +1,3 @@
 invoke
 packaging
+pre-commit


I feel all the pre-commit hooks should be moved to a separate PR; it'll make things easier to review. :)

Okay, I can do this.

pradyunsg · 2018-04-20T07:23:04Z

.travis.yml

+- "2.6"
+
+.helpers:
+- &reset_steps


I'd rather not use YAML magic and have a verbose .travis.yml instead.

It's not magic, lot's of people use it and it really helps to not get lost in copy-pasted things, thus I'm very opinionated about this.

pradyunsg · 2018-04-20T07:26:14Z

Thanks for this @hugovk! :)

It'd be nice if the pre-commit hooks (and related cleanups) would be moved to another PR and only the CI check for basic installation is kept in this PR.

It'll be easier to review both those PRs then.

hugovk · 2018-04-20T07:54:57Z

Hi @webknjaz, do you think you'd be able to split this up? Perhaps two new PRs starting from this branch, or whatever's easiest. Thank you!

webknjaz · 2018-04-20T11:33:13Z

I think, it might be better to accept pre-commit change first and go for travis stuff then.

webknjaz · 2018-04-20T12:44:28Z

@hugovk It turned out I've already put everything in a single commit :)

webknjaz · 2018-04-20T13:05:01Z

@pradyunsg do you want me to cherry-pick linter fixes to #28 also?

pradyunsg · 2018-04-21T09:35:42Z

@pradyunsg do you want me to cherry-pick linter fixes to #28 also?

Sure!

webknjaz · 2018-04-21T12:27:26Z

done

webknjaz · 2018-04-23T12:51:04Z

@hugovk would you mind to rip off linting-related commits from this branch, so that it would be easier for @pradyunsg to accept it faster?

webknjaz · 2018-04-23T14:13:27Z

@hugovk I believe linter fixes should also go away

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

hugovk · 2018-04-24T13:22:46Z

Rebased on master (and removed the last two unnecessary revert and regenerate commits).

pfmoore · 2018-04-24T15:02:22Z

#31 is more like what I had in mind for a test suite (I've still to add Travis and Appveyor config for it).

webknjaz · 2018-04-24T15:28:06Z

It looks interesting. I'll review it more closely later today. But maybe we could accept this PR and then add yours on top of it or vice versa?

pfmoore · 2018-04-24T16:19:16Z

.travis.yml

+
+env:
+- SCRIPT_SRC=local
+- SCRIPT_SRC=remote


I'm not at all sure that we should be testing "remote" here. The tests should be testing what's in the repository - for example, the test runs for a PR shouldn't fail because the published version was broken.

Good point, will remove it.

We can move tests for remote to run only when triggered manually (or via API) and not PR. Deal? I don't want to completely eliminate them.

Not sure I care much, TBH. As noted, I'm against this approach (of using a shell script) anyway.

pfmoore · 2018-04-24T16:20:22Z

.travis/helpers.sh

+  fi
+
+  >&2 echo Version $1 requires following script: $path
+  $cmd $path


Are you testing an install into an environment that currently doesn't have pip installed? I can't tell (but I don't think you are)... Someone who has pip installed can simply do python -m pip install -U pip. (Sure, we have to support that case, but the key use case is someone trying to bootstrap pip into an empty environment).

Yes, pip is uninstalled at the start: https://github.com/hugovk/get-pip/blob/c7bbeb9da01074f9f5b1ec0fedd1273aee8460dc/.travis.yml

OK, thanks for the pointer.

pfmoore · 2018-04-24T17:00:42Z

See #18 (comment) for more detailed comments. I personally don't think the changes here are the right way to go.

webknjaz reviewed Apr 18, 2018

View reviewed changes

hugovk force-pushed the travis-ci branch from 022c6ce to a24ded1 Compare April 18, 2018 13:01

webknjaz reviewed Apr 18, 2018

View reviewed changes

hugovk force-pushed the travis-ci branch from abe1e3c to c0cd3e9 Compare April 18, 2018 13:32

webknjaz approved these changes Apr 18, 2018

View reviewed changes

pradyunsg requested changes Apr 20, 2018

View reviewed changes

webknjaz mentioned this pull request Apr 20, 2018

Integrate pre-commit tool #28

Closed

hugovk and others added 19 commits April 24, 2018 16:17

Add -uo pipefail

6bcff56

Remove redundant trailing semicolons

6509181

Add space after hashbang

e134ded

Unify test scripts and split into local/remote jobs

6ac9ecb

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

Only flake8 on a recent Python

905498f

Update build matrix

33ce842

Move test helpers to .travis dir

fdead1d

Make helper select correct version

7814be9

Run helper script instead of sourcing it

543ef4c

Move linters to separate stage

ce9fc7b

Reset unnecessary inherited options

e9e3642

Integrate pre-commit tool

5de1003

Enable debug in helper script

fef3e5e

Output env vars in case of job fail

09dd8c6

Uninstall pip before testing

d287e5c

Show pip version after installation

58d7c52

Enable pre-commit and dists caching

3c940ef

Remove linting

123db6b

pip install --yes

c7bbeb9

hugovk force-pushed the travis-ci branch from c16928b to c7bbeb9 Compare April 24, 2018 13:19

pfmoore reviewed Apr 24, 2018

View reviewed changes

pfmoore requested changes Apr 24, 2018

View reviewed changes

hugovk mentioned this pull request Apr 25, 2018

[WIP] Add a very basic test suite #31

Closed

hugovk closed this Aug 9, 2019

hugovk deleted the travis-ci branch August 9, 2019 08:12

webknjaz mentioned this pull request Sep 17, 2019

Add GitHub Actions CI integration #48

Merged

Simple install tests on Travis CI #23

Simple install tests on Travis CI #23

Conversation

hugovk commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz commented Apr 18, 2018

webknjaz Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

webknjaz commented Apr 18, 2018

webknjaz commented Apr 18, 2018

webknjaz commented Apr 18, 2018

hugovk commented Apr 18, 2018 • edited Loading

webknjaz commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg commented Apr 20, 2018

hugovk commented Apr 20, 2018

webknjaz commented Apr 20, 2018 • edited Loading

webknjaz commented Apr 20, 2018

webknjaz commented Apr 20, 2018

pradyunsg commented Apr 21, 2018

webknjaz commented Apr 21, 2018

webknjaz commented Apr 23, 2018

webknjaz commented Apr 23, 2018

hugovk commented Apr 24, 2018

pfmoore commented Apr 24, 2018

webknjaz commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfmoore commented Apr 24, 2018

webknjaz Apr 18, 2018 •

edited

Loading

webknjaz Apr 18, 2018 •

edited

Loading

hugovk commented Apr 18, 2018 •

edited

Loading

webknjaz commented Apr 20, 2018 •

edited

Loading