Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server fixture #812

Merged
merged 6 commits into from
Nov 15, 2016
Merged

Server fixture #812

merged 6 commits into from
Nov 15, 2016

Conversation

jgosmann
Copy link
Collaborator

This automatically starts the nengo_gui server for tests with a fixture. It allows to run the tests with a simple py.test and no need to start a server beforehand.

It also automatically selects a free port allowing to run another nengo_gui server or other things on the default things in parallel (e.g. to work on an actual model while the tests run).

@mention-bot
Copy link

@jgosmann, thanks for your PR! By analyzing the annotation information on this pull request, we identified @rsimmons1, @tbekolay and @tcstewar to be potential reviewers

@Seanny123
Copy link
Collaborator

LGTM.

@tbekolay
Copy link
Member

This is a big improvement! One thing I noticed in the Travis build is that the selenium and GUI servers weren't closed and had to be killed. I ran this locally twice, and in once case Firefox remained open (which I assume means the selenium server was still up) and once it closed. This SO thread indicates that we should be using driver.quit(), so I made that change (9e6f47a)... for shutting down the Nengo GUI server perhaps we should use a longer timeout than 50 milliseconds? Possibly no timeout? I think it's better to wait a bit for the server to shut down properly so resources are freed.

@jgosmann
Copy link
Collaborator Author

  • We should also remove the nengo command from .travis.yml to not start two servers.
  • No timeout might block forever when a thread completely hangs. I took 50 milliseconds because it is what the interactive GUI uses and it's usually enough to have all threads end normally without introducing too much delay. But I'm fine with changing that number within reasonable bounds.

@tbekolay tbekolay force-pushed the server-fixture branch 2 times, most recently from 240645f to 59f4e62 Compare July 28, 2016 20:11
@tbekolay
Copy link
Member

tbekolay commented Jul 28, 2016

I made some commits to fix up the .travis.yml. The Py2 build is about the same time as the old one, but doesn't require sudo, so should start up faster than the current build process (plus sudo builds will be disabled eventually). And doesn't forcibly kill the selenium or nengo GUI servers. The Py3 build fails for issues that are probably pretty easy to fix.

Should I make a separate PR for the TravisCI stuff and those fixes or keep doing here?

@Seanny123
Copy link
Collaborator

I think those changes are necessary to make the changes functional,
consequently I would keep making those changes here.

On Thu, Jul 28, 2016 at 4:31 PM, Trevor Bekolay notifications@github.com
wrote:

I made some commits to fix up the .travis.yml. The Py2 build
https://travis-ci.org/nengo/nengo_gui/jobs/148138198 is about the same
time as the old one, but doesn't require sudo, so should start up faster
than the current build process (plus sudo builds will be disabled
eventually). The Py3 build
https://travis-ci.org/nengo/nengo_gui/jobs/148138199 fails for issues
that are probably pretty easy to fix.

Should I make a separate PR for the TravisCI stuff and those fixes or keep
doing here?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#812 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB1HaRZaLdKvKb4tPrUqW_dBTAKfx2G_ks5qaRGPgaJpZM4JTPr3
.

@tbekolay
Copy link
Member

I think those changes are necessary to make the changes functional

The changes make local testing better and still pass in TravisCI, so definitely not necessary. But yeah, it also shouldn't take much to fix either so I'll look at it soon (just didn't want to hold up this PR if @tcstewar wanted to merge).

@tbekolay
Copy link
Member

tbekolay commented Aug 2, 2016

Since this PR hasn't moved I fixed up the Python 3 style, so (fingers crossed) this should now pass on Python 2 and 3.

@Seanny123
Copy link
Collaborator

Awesome changes Trevor! I liked reviewing this pull request because it taught me a lot about TravisCI for Python. LGTM.

@tbekolay
Copy link
Member

tbekolay commented Nov 3, 2016

Given the decision at the last dev meeting we should really get this PR merged @tcstewar as it's a big improvement to the testing infrastructure.

@tcstewar
Copy link
Collaborator

tcstewar commented Nov 4, 2016

Hmm, I get a seg fault, and it seems to require py.test==2.9.1 for me.... That could just be my computer having some broken things in it.... hmm.... I've got a few things I can try, though.

Here's the not-all-that-informative seg fault (after firefox pops up and briefly shows the network...):

tcstewar@ctn13 ~/github/nengo_gui/nengo_gui/tests $ py.test
============================= test session starts ==============================
platform linux2 -- Python 2.7.6, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /home/tcstewar/github/nengo_gui, inifile: 
collected 41 items 

test_basic_functionality.py Segmentation fault

@tcstewar
Copy link
Collaborator

tcstewar commented Nov 4, 2016

Never mind, it's just my computer -- I get the same seg fault in master. Grr....

@tbekolay
Copy link
Member

tbekolay commented Nov 4, 2016

Are you using a virtual environment? If so, that can happen if you've updated Python. Easiest fix is to wipe your virtual environments and remake them.

@tcstewar
Copy link
Collaborator

tcstewar commented Nov 4, 2016

Grr... same problem with a completely fresh virtual environment.... :(

@tbekolay
Copy link
Member

tbekolay commented Nov 4, 2016

I think I had that problem once with sqlite not being linked to Python? Or maybe it was Tcl? In any case, probably have to reinstall Python :(

@jgosmann
Copy link
Collaborator Author

jgosmann commented Nov 4, 2016

You can see whether the faulthandler package gives you some more information on what is causing the segfault.

jgosmann and others added 4 commits November 15, 2016 10:56
This makes the test slightly faster.

Also switch to driver.quit(), as driver.close() just closes the
current web browser window.
Since we now use a session scoped fixture, we should
shut down the whole selenium server after the test session.
This allows to run all tests with a simple `py.test nengo_gui`
without the need to start the server manually. It also automatically
selects a free port allowing to run another nengo_gui server or other
things on the default things in parallel (e.g. to work on an actual
model while the tests run).
While much of this is to pass flake8 checks, in some instances
there were tabs in these files, which Python 3 requires you to
do very carefully. Since some lines had mixed tabs and spaces,
the tests wouldn't run in Python 3. With these style fixes, tests
run and pass in Python 3.
This makes the Nengo GUI TravisCI build more similar to Nengo's.
A few important differences:

- Tests on both Python 2 and 3.
- Uses Miniconda instead of the built-in Python
  (should have a faster NumPy than the built-in).
- Doesn't use `sudo` (allows us to use container infrastructure,
  which should make starting up builds much faster).
- Doesn't start Nengo GUI server twice.
- Doesn't install Chrome webdriver (since it's not used -- selenium
  comes with the Firefox driver and TravisCI has Firefox installed).
@tbekolay
Copy link
Member

I took a look at this PR again since we talked about it a while back. I rebased it to master and cleaned up the history, but it failed on TravisCI because Selenium recently released version 3.0, which has a whole different Firefox webdriver. I tried to get that working, but it just wasn't happening (will make a separate PR for that), so I changed .travis.yml to stick with 2.53 for now. Even with that, test_spa.py was failing in Python 2 (has been for a while I think, though it works intermittently and usually in Python 3). Added a sleep to fix that and now the tests are passing on TravisCI again. They pass locally too (though not with Selenium 3).

Anyway, it's worth merging now as it's a big improvement over the current state of things. I'll look into getting Selenium 3 working after lunch.

@tcstewar
Copy link
Collaborator

Sounds good. Thanks for digging into this (I still haven't got it running on my machine, which is I think an indication that it's time to reinstall, as I'm pretty sure I've broken a bunch of things attempting to fix this)... I'll bring this into master now. :)

@tcstewar tcstewar merged commit b892f49 into master Nov 15, 2016
@tbekolay tbekolay deleted the server-fixture branch October 17, 2017 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants