PR: Upgrade to CircleCI v2.0 #159

jitseniesen · 2018-08-22T19:53:17Z

Fixes #156

jitseniesen · 2018-08-22T21:36:12Z

The tests segfault on Circle, but they also segfault when I run them locally, so it looks like the CircleCI script works fine and something else broke in the last half year.

jitseniesen · 2018-08-22T21:53:03Z

I tried to create an environment similar to that of half a year ago with

conda install qt=5.6.2 spyder=3.2.6 ipython=6.2.1 notebook=5.4.0 qtpy=1.3.1

but the test still segfaults locally.

jitseniesen · 2018-08-23T13:54:11Z

Tests are passing now on Circle CI after I removed pytest-xvfb (which was installed via pip) and also when run locally on one of my computers, so this is ready to be merged.

The tests do segfault when run on another computer but I'm assuming that is a local issue.

ccordoba12 · 2018-08-23T15:53:50Z

I have a couple of things to say about this. Please don't merge it yet.

ccordoba12 · 2018-08-23T19:31:57Z

.circleci/config.yml

+            wget https://repo.continuum.io/miniconda/Miniconda3-latest-Linux-x86_64.sh -O miniconda.sh
+            bash miniconda.sh -b -p $HOME/miniconda
+            export PATH="$HOME/miniconda/bin:$PATH"
+            conda install python=$PYTHON_VERSION


In this step:

Please move these commands to an install.sh script in .circleci

Please use Astropy's ci-helpers instead of installing Miniconda by hand. That's what we have adopted is most other repos in our org. See for example qtpy's install.sh.

ccordoba12 · 2018-08-23T19:32:46Z

.circleci/config.yml

+            export PATH="$HOME/miniconda/bin:$PATH"
+            conda install python=$PYTHON_VERSION
+      - run:
+          name: Install Spyder from GitHub


There's no need to have this step now. It was needed before we released Spyder 3.2, but not anymore, so please remove it.

@ccordoba12 I am not sure I understand what you are saying. Do you mean that it is easier to install spyder via conda instead of via github? That is true, but I think it is better to test against the version at the head of the 3.x branch on git, in order to catch any regressions. I know that 3.x is not supposed to introduce any backward incompatibilities, but it is good to test for it. Of course, this does mean testing against a moving target which has its own problems, but I felt this approach was the most useful.

I know that 3.x is not supposed to introduce any backward incompatibilities, but it is good to test for it

Ok. I was thinking to test against the latest release, but sure, we can test against the HEAD of 3.x

Then please move this to install.sh too.

ccordoba12 · 2018-08-23T19:33:16Z

.circleci/config.yml

+            cd spyder-3.x
+            python setup.py install
+      - run:
+          name: Set up test environment


The commands in this step can go to install.sh too.

And they will be simpler after switching to ci-helpers.

ccordoba12 · 2018-08-23T19:34:09Z

.circleci/config.yml

+            echo '********** output of conda list **********'
+            conda list
+      - run:
+          name: Run tests


Please move these commands to a script called run_tests.sh. You can find an example in qtpy too.

ccordoba12 · 2018-08-23T19:34:47Z

.circleci/config.yml

+          command: |
+            export PATH="$HOME/miniconda/bin:$PATH"
+            mkdir test-reports
+            pytest spyder_notebook --cov=spyder_notebook --junitxml=test-reports/junit.xml


This command needs to be

pytest -x -vv spyder_notebook --cov=spyder_notebook

And after this command please add these lines

if [ $? -ne 0 ]; then exit 1 fi

so that this step fails if the pytest exit code is not 0.

ccordoba12 · 2018-08-23T19:35:19Z

.circleci/config.yml

+            export PATH="$HOME/miniconda/bin:$PATH"
+            mkdir test-reports
+            pytest spyder_notebook --cov=spyder_notebook --junitxml=test-reports/junit.xml
+            COVERALLS_REPO_TOKEN=Kr503QwklmJYKXYRXLywrtw8zbX7K8SKx coveralls


There's no need to use COVERALLS_REPO_TOKEN in Circle now.

Without the token, I get the following error:

Couldn't find a repository matching this job. Traceback (most recent call last): File "/home/circleci/miniconda/bin/coveralls", line 11, in <module> sys.exit(main()) File "/home/circleci/miniconda/lib/python3.6/site-packages/coveralls/cli.py", line 80, in main log.info(result['url']) KeyError: 'url' Exited with code 1

ccordoba12 · 2018-08-23T19:36:23Z

.circleci/config.yml

+            pytest spyder_notebook --cov=spyder_notebook --junitxml=test-reports/junit.xml
+            COVERALLS_REPO_TOKEN=Kr503QwklmJYKXYRXLywrtw8zbX7K8SKx coveralls
+      - run:
+          name: Run style checks


This can be removed. Unfortunately ciocheck hasn't been updated in a long time.

You could set up support for pep8speaks in another PR though.

ccordoba12 · 2018-08-23T19:42:31Z

Thanks for doing this @jitseniesen! My main suggestion is to use shell scripts to perform the installation and testing steps. That way things will be more readable and maintainable than putting everything in a big config.yml.

Also, we should use ci-helpers everywhere from now on. It's a fantastic project and it works pretty well too.

jitseniesen · 2018-08-24T16:17:46Z

Also, we should use ci-helpers everywhere from now on. It's a fantastic project and it works pretty well too.

When I was testing, I found ci-helpers not so helpful because I didn't understand what is going on. When I looked into it, it seemed that only six lines of script was relevant for me, so I thought it better to just include them explicitly. But if you prefer to use ci-helpers, I'll give it another go.

ccordoba12 · 2018-08-24T17:03:52Z

But if you prefer to use ci-helpers, I'll give it another go.

Please do. Just follow the example of qtpy.

ccordoba12 · 2018-08-24T17:13:53Z

The nice thing about ci-helpers is that you declare your dependencies through the env vars CONDA_DEPENDENCIES and PIP_DEPENDENCIES. That's what makes things more maintainable.

ccordoba12 · 2018-08-24T17:14:18Z

The latest error is caused because the scripts are not executable files.

goanpeca · 2018-08-24T17:22:20Z

Hi Jit thanks a lot for working on this :-)

I have mixed views on the recommendation of using ci helpers vs raw install commands for setting the environment. I originally used ci helpers and even I believe I advicated the use of them, but I have found that solution to be limiting under certain scenarios to the point that I rollback most projects where I used cihelpers back to a similar way like implemented on this PR.

It does make the yaml files simpler but it also obfuscates what is going on.

ccordoba12 · 2018-08-24T17:27:22Z

It does make the yaml files simpler but it also obfuscates what is going on.

Sure, but you usually need to touch ci-helpers env vars. But ok, if you prefer to not use it here, I'm not opposed to the idea.

@jitseniesen, last recommendation then: please unify all install-foo.sh scripts into a single install.sh one. I don't think we need three separate scripts for that.

ccordoba12 · 2018-08-25T14:19:21Z

.circleci/install.sh

+popd
+
+# Install spyder-notebook from source in developer mode
+python setup.py develop


This line is what's causing the error in Circle. It's really not necessary, so please remove it.

ccordoba12 · 2018-08-25T14:24:02Z

.circleci/run-tests.sh

+
+# Run tests; flag -x means stop on first test failure 
+pytest -x -vv spyder_notebook --cov=spyder_notebook --junitxml=test-reports/junit.xml
+


As I told you before, please add here this bit of shell code

if [ $? -ne 0 ]; then exit 1 fi

to make this step really fail in Circle. That checks if the exit code of the previous step is 0, and if not, it forces an exit 1.

ccordoba12 · 2018-08-25T14:24:31Z

.circleci/run-tests.sh

+
+export PATH="$HOME/miniconda/bin:$PATH"
+conda activate test
+mkdir test-reports


This line shouldn't be necessary. Please try to remove it.

ccordoba12 · 2018-08-25T14:26:28Z

.circleci/install.sh

+
+# Install Miniconda and required packages using astropy's ci-helpers
+TRAVIS_OS_NAME='linux'
+CONDA_DEPENDENCIES='notebook pytest pytest-cov flaky'


Please add below this step

CONDA_DEPENDENCIES_FLAGS="--quiet"

to avoid a lot of unnecessary noise in this step.

ccordoba12 · 2018-08-25T14:26:45Z

.circleci/install.sh

+TRAVIS_OS_NAME='linux'
+CONDA_DEPENDENCIES='notebook pytest pytest-cov flaky'
+PIP_DEPENDENCIES='coveralls pytest-qt'
+git clone --depth 1 git://github.com/astropy/ci-helpers.git


Please add blank above this line.

ccordoba12 · 2018-08-25T14:27:46Z

.circleci/config.yml

+      - store_test_results:
+          path: test-reports
+      - store_artifacts:
+          path: test-reports


I see test-reports is used here. Is this really necessary?

I think this uploads the test results to CircleCI but it is not clear what they do with it, so I'll remove this.

jitseniesen · 2018-08-25T15:47:33Z

But ok, if you prefer to not use [astropy's ci-helpers] here, I'm not opposed to the idea.

I tried to use it for consistency with the other repos, but I found that it downgrates conda to 4.3. That version does not have the --only-deps flag which is really quite useful so I decided against using ci-helpers.

This commit is for testing purposes and will be reverted in the next commit.

This reverts commit b07b54a.

jitseniesen · 2018-08-25T15:58:21Z

I checked and it is not necessary to add

if [ $? -ne 0 ]; then
    exit 1
fi

after the call to pytest, because the script is run with the -e flag.

This is now ready for merging as far as I am concerned.

ccordoba12 · 2018-08-25T16:30:52Z

.circleci/install.sh

+export PATH="$HOME/miniconda/bin:$PATH"
+
+# Install required Python version
+conda install -y python=$PYTHON_VERSION


You're installing everything in conda's base or root environment.

That's bad. Instead, please create a clean env (ci-helpers calls it test) and install everything there.

ccordoba12

This is ready, at last!

jitseniesen added 2 commits August 22, 2018 20:41

Upgrade to CircleCI v2.0

cda7011

Use pip to install pytest-xvfb

c8828aa

jitseniesen added this to the v0.1.3 milestone Aug 22, 2018

jitseniesen added 4 commits August 23, 2018 12:49

Add 'conda list' to help debugging

69825ff

Remove pytest-xvfb

735bd8d

Add: pip install coveralls; turn Python 2.7 and 3.7 back on

0bfffa7

Fix typo in ciocheck command

fe10a91

ccordoba12 reviewed Aug 23, 2018

View reviewed changes

Remove ciocheck from Circle CI check because no longer active

c58865b

Split CircleCI's config.yml file in smaller script files

2bbea00

Make shell files executable

df3d349

jitseniesen added 2 commits August 25, 2018 10:21

Merge CircleCI's install-xx.sh scripts into one script

771c6cb

Add -y flag to conda install

91ba2ff

Add -x -vv flags to pytest invocation

f5f0ac8

jitseniesen force-pushed the circle2 branch from f43182a to f5f0ac8 Compare August 25, 2018 12:28

ccordoba12 reviewed Aug 25, 2018

View reviewed changes

jitseniesen added 3 commits August 25, 2018 16:40

Add -x flag to bash in CircleCI scripts

bdde730

Do not upload test results on CircleCI

743ac84

Remove 'python setup.py develop' from CircleCI's installation script

7915294

jitseniesen force-pushed the circle2 branch from 9278ea1 to 7915294 Compare August 25, 2018 15:42

jitseniesen added 2 commits August 25, 2018 16:48

Add failing test to check that CI will pick this up

b07b54a

This commit is for testing purposes and will be reverted in the next commit.

Revert "Add failing test to check that CI will pick this up"

d9f6a81

This reverts commit b07b54a.

ccordoba12 reviewed Aug 25, 2018

View reviewed changes

jitseniesen added 2 commits August 25, 2018 18:45

Create new conda environent in CircleCI script for running tests

81b6657

Use new command to set up conda in CircleCI script

f497976

ccordoba12 changed the title ~~Upgrade to CircleCI v2.0~~ PR: Upgrade to CircleCI v2.0 Aug 25, 2018

ccordoba12 approved these changes Aug 25, 2018

View reviewed changes

ccordoba12 merged commit fa3f04c into spyder-ide:master Aug 25, 2018

jitseniesen deleted the circle2 branch August 25, 2018 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Upgrade to CircleCI v2.0 #159

PR: Upgrade to CircleCI v2.0 #159

jitseniesen commented Aug 22, 2018

jitseniesen commented Aug 22, 2018

jitseniesen commented Aug 22, 2018

jitseniesen commented Aug 23, 2018

ccordoba12 commented Aug 23, 2018

ccordoba12 Aug 23, 2018

ccordoba12 Aug 23, 2018

jitseniesen Aug 24, 2018

ccordoba12 Aug 24, 2018

ccordoba12 Aug 23, 2018

ccordoba12 Aug 23, 2018

ccordoba12 Aug 23, 2018

ccordoba12 Aug 23, 2018

ccordoba12 Aug 23, 2018

ccordoba12 Aug 23, 2018

jitseniesen Aug 25, 2018

ccordoba12 Aug 23, 2018

ccordoba12 commented Aug 23, 2018

jitseniesen commented Aug 24, 2018

ccordoba12 commented Aug 24, 2018

ccordoba12 commented Aug 24, 2018

ccordoba12 commented Aug 24, 2018

goanpeca commented Aug 24, 2018

ccordoba12 commented Aug 24, 2018

ccordoba12 Aug 25, 2018

ccordoba12 Aug 25, 2018

ccordoba12 Aug 25, 2018

ccordoba12 Aug 25, 2018

ccordoba12 Aug 25, 2018

ccordoba12 Aug 25, 2018

jitseniesen Aug 25, 2018

jitseniesen commented Aug 25, 2018

jitseniesen commented Aug 25, 2018

ccordoba12 Aug 25, 2018

ccordoba12 left a comment


		# Run tests; flag -x means stop on first test failure
		pytest -x -vv spyder_notebook --cov=spyder_notebook --junitxml=test-reports/junit.xml

PR: Upgrade to CircleCI v2.0 #159

PR: Upgrade to CircleCI v2.0 #159

Conversation

jitseniesen commented Aug 22, 2018

jitseniesen commented Aug 22, 2018

jitseniesen commented Aug 22, 2018

jitseniesen commented Aug 23, 2018

ccordoba12 commented Aug 23, 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

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

ccordoba12 commented Aug 23, 2018

jitseniesen commented Aug 24, 2018

ccordoba12 commented Aug 24, 2018

ccordoba12 commented Aug 24, 2018

ccordoba12 commented Aug 24, 2018

goanpeca commented Aug 24, 2018

ccordoba12 commented Aug 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

jitseniesen commented Aug 25, 2018

jitseniesen commented Aug 25, 2018

Choose a reason for hiding this comment

ccordoba12 left a comment

Choose a reason for hiding this comment