Add travis-based integration tests #146

rentzso · 2016-02-03T01:02:30Z

@josschne Creating this PR to track work on enabling integration testing in travis

josschne · 2016-02-09T22:57:22Z

Ping @gsfr / @rentzso . I think we're ready to go with this. Please take a look.

Note that coverage through paster for the integration tests is a bit tricky. If we want to get those numbers we probably will need to refactor the integration tests to call the WSGI handler directly, see an example here: http://stackoverflow.com/questions/6222528/unittesting-the-webapp-requesthandler-in-gae-python

gsfr · 2016-02-10T04:14:40Z

bin/run.sh

-
-
-# Default config values
+SCITRAN_CORE_INSECURE=${SCITRAN_CORE_INSECURE:-"true"}


This seems like a needlessly dangerous default. Couldn't we just have Travis set the environment variable?

Yep, we could do that. I thought this would be easier for newbies - the tests fail without this setting.

I see. Why don't we have runtests.sh set this to true, but not run.sh itself?

gsfr · 2016-02-10T04:37:58Z

Thanks, @josschne.

Would using uWSGI help with the coverage issue?

Tentatively LGTM, pending clarifications or minor updates on various line comments.

@andynemzek: Would appreciate a quick review of the diff on the two .sh files.

josschne · 2016-02-10T15:02:37Z

Don't think using uWSGI would help with the coverage issue. In either case we need to enable coverage within the python environment, which can be done but is non-trivial. I'd like to focus on getting unit test coverage up and use the integration tests for their functional value, rather than any coverage.

kofalt · 2016-02-10T16:11:35Z

.travis.yml

+# to install python manually
+# Python isn't supported OOTB, so pick a language that makes Travis happy
+language: objective-c
+os: osx


Why are we testing on OSX?
At most I'd expect us to matrix build against both linux and OSX.

This script assumes OSX - it uses brew. Would be nice to support multiple environments, but my understanding was we were limiting to OSX to start. Is that correct @gsfr?

That is correct. We wanted to get started with Travis asap and we had an OS X launcher.

andynemzek · 2016-02-10T16:17:28Z

@gsfr didn't see anything pop out at me, but run.sh has grown quite a bit since I last worked on it.

kofalt · 2016-02-10T16:17:42Z

.travis.yml

+  - brew update
+  - brew install python
+before_script:
  - bin/install.sh --ci


install.sh does not appear to take a --ci flag.

Not yet... :)

kofalt · 2016-02-10T16:26:50Z

I'm going to stop reviewing here, as it started to become clear that much of the script's non-portability pre-dates this PR. I played with it a bit, but... for now I'll just accept that I can't run the tests this way, and maybe we worry about that (and CI on linux) after this lands.

kofalt · 2016-02-22T16:35:06Z

@gsfr @josschne What are the remaining blockers on getting this merged? No activity in awhile... I see one unanswered question (the first thread from the top).

gsfr · 2016-02-22T18:20:40Z

Thanks for the ping, @kofalt. Joe and I are on it. No action needed at the moment.

ryansanford · 2016-03-07T18:04:57Z

@gsfr @josschne

Looks like we could extend docker-compose.yml to include nginx to support https out of the box for this runtests entrypoint. Thoughts?

kofalt · 2016-03-07T21:02:06Z

.travis.yml

+  - sudo apt-get update -qq
+  - sudo apt-get -o Dpkg::Options::="--force-confnew" install -y -q docker-engine
+  - sudo curl -o /usr/local/bin/docker-compose -L https://github.com/docker/compose/releases/download/1.6.2/docker-compose-`uname -s`-`uname -m`
+  - sudo chmod +x /usr/local/bin/docker-compose


In general I prefer to keep the lines of vendor-specific CI files sparse, and instead throw these lines in a script where it's easier to document, maintain, and understand. bin/ci-install-docker-ecosystem.sh?

Not a blocker for merge.

kofalt · 2016-03-07T21:06:24Z

And I'd like @kofalt's once-over just cause he's awesome

😎

This looks pretty great. At the architecture level, I question if we really need to boot virtualbox... I would have assumed we could use Travis' docker infra and just ran our containers on it. I'm assuming you've either already been down that road or decided it wasn't worth it. Either way, this approach is A-OK since it gets us that delicious green checkmark on PRs.

The test layout is a tad hairy but makes sense; I like the fixtures. Great work!

My read on the uwsgi config is that it probably won't affect us since we're using UWSGI in fast-cgi mode in prod, so I presume keepalive settings don't apply. Could be wrong!

josschne · 2016-03-07T22:07:38Z

@ryansanford @kofalt: Thanks for the review!

We only need to boot virtualbox on OSX (via docker-machine). The Travis CI tests are now running on Linux and assume docker is ready (Travis CI's OSX infra actually can't run virtualbox - which is where I got stuck).

Not sure on the uwsgi comments. @ryansanford thoughts? What's the best way to get the keepalive options into the mix without breaking prod?

@ryansanford I don't think http nor nginx are necessary here. I'm not sure I understood the motivation behind your comment.

gsfr · 2016-03-08T02:17:51Z

Thanks, @josschne. Nice work!

I would like to still understand @ryansanford's comments before merging.

Also, we may want to bring this in line with the conflicting #185 before merging.

josschne · 2016-03-09T07:20:08Z

Ping @ryansanford. Can you provide more detail on your nginx comment?

@gsfr - Looks like integration with #185 could be a large change to this PR. Would it make sense to separate that from this PR?

gsfr · 2016-03-09T19:27:48Z

Thanks, @josschne. After team discussion, we decided to go ahead with merging #185, which just happened. @ryansanford has committed to update this PR to fall inline with #185 as well as his latest infra.

josschne · 2016-03-09T20:42:09Z

@gsfr - Alright. I'll consider my part on this PR complete.

kofalt · 2016-03-09T20:43:09Z

👍 😄 This is great stuff Joe, thanks!

coveralls · 2016-03-18T20:17:28Z

Coverage remained the same at 8.008% when pulling 9118332 on integration-testing into fe7737d on master.

coveralls · 2016-03-21T11:53:13Z

Coverage remained the same at 8.008% when pulling 65e1553 on integration-testing into fe7737d on master.

ryansanford · 2016-03-21T12:01:25Z

@gsfr Conflicts should be resolved, and travis build completes without error. Any final words before merging?

gsfr · 2016-03-21T18:59:07Z

Thanks, @ryansanford. Let's have @rentzso and @nagem take a final look.

rentzso · 2016-03-21T21:06:02Z

LGTM

nagem · 2016-03-22T23:04:21Z

LGTM

gsfr mentioned this pull request Feb 3, 2016

Added integration test for BIDS downloads #145

Closed

josschne force-pushed the integration-testing branch 7 times, most recently from 108aca3 to 2e88c16 Compare February 9, 2016 22:56

gsfr changed the title ~~adapt tests to the paster app server~~ Add travis-based integration tests Feb 10, 2016

gsfr reviewed Feb 10, 2016
View reviewed changes

kofalt reviewed Feb 10, 2016
View reviewed changes

josschne changed the title ~~Add travis-based integration tests~~ Add travis-based integration tests - OSX only Feb 11, 2016

gsfr force-pushed the master branch 2 times, most recently from aaf2936 to d5f9db2 Compare February 26, 2016 22:00

josschne force-pushed the integration-testing branch 3 times, most recently from 0f93b08 to b18f029 Compare March 1, 2016 06:44

josschne force-pushed the integration-testing branch from bc5d1e2 to c2489e8 Compare March 5, 2016 20:54

kofalt reviewed Mar 7, 2016
View reviewed changes

gsfr mentioned this pull request Mar 8, 2016

Remove data bootstrapping; add folder reaping #185

Merged

ryansanford self-assigned this Mar 18, 2016

Renzo Frigato and others added 7 commits March 18, 2016 14:53

adapt tests to the paster app server

bf37139

Update run.sh to support daemonization for CI testing

ff409e3

WIP on integration tests in Docker

dadc0c7

Remove daemonized run

38cdac4

Add docker-compose dependency

0d79194

Make changes to work on master, BF downloads

9c5e1fa

Update code comment from PR review

9118332

ryansanford force-pushed the integration-testing branch from d7575fd to 9118332 Compare March 18, 2016 20:12

Support latest bootstrapping requirements

65e1553

rentzso merged commit 2aa9153 into master Mar 22, 2016

rentzso deleted the integration-testing branch March 22, 2016 23:07



		# Default config values
		SCITRAN_CORE_INSECURE=${SCITRAN_CORE_INSECURE:-"true"}

Add travis-based integration tests #146

Add travis-based integration tests #146

Uh oh!

Conversation

rentzso commented Feb 3, 2016

Uh oh!

josschne commented Feb 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsfr commented Feb 10, 2016

Uh oh!

josschne commented Feb 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andynemzek commented Feb 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kofalt commented Feb 10, 2016

Uh oh!

kofalt commented Feb 22, 2016

Uh oh!

gsfr commented Feb 22, 2016

Uh oh!

ryansanford commented Mar 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kofalt commented Mar 7, 2016

Uh oh!

josschne commented Mar 7, 2016

Uh oh!

gsfr commented Mar 8, 2016

Uh oh!

josschne commented Mar 9, 2016

Uh oh!

gsfr commented Mar 9, 2016

Uh oh!

josschne commented Mar 9, 2016

Uh oh!

kofalt commented Mar 9, 2016

Uh oh!

coveralls commented Mar 18, 2016

Uh oh!

coveralls commented Mar 21, 2016

Uh oh!

ryansanford commented Mar 21, 2016

Uh oh!

gsfr commented Mar 21, 2016

Uh oh!

rentzso commented Mar 21, 2016

Uh oh!

nagem commented Mar 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants