Skip to content

Add keepalive for root connection#4617

Merged
jburel merged 2 commits intoome:developfrom
ximenesuk:test-root-keepalive
May 2, 2016
Merged

Add keepalive for root connection#4617
jburel merged 2 commits intoome:developfrom
ximenesuk:test-root-keepalive

Conversation

@ximenesuk
Copy link
Copy Markdown
Contributor

This an attempt to fix the three fails record on the card https://trello.com/c/EavoXnp4/649-importas-and-multigroup-cli-tests

This adds a keepAlive for the root connection so that the connection is still available when these three tests need it. This is the most minimal solution and appears to work when testing locally against a remote server - it needs this latency for the tests to take long enough for the connection to be lost, see below.

Testing

The tests on Jenkins should pass, though given the intermittency the tests will need to be checked for a number of runs.

Local testing is possible though dependent on a slower connection. Set ice.config to point at eel.openmicroscopy.org:24064 and then:

./setup.py test -t test/integration/clitest/test_import.py -m "not broken" -k "not Symlink" -v

should pass cleanly.

"""
try:
if self.root.sf is None:
assert self.root.connect()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is root here a BlitzGateway or a BaseClient? In the latter case, I don't think there's a "connect" method. NB: an alternative option would be enableKeepAlive on the BaseClient but that will require cleaning them up, otherwise tests may hang.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, this is bad pasting from me and because nothing failed not looking further.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course this also means this condition was never true and so is just a simple keepalive (the else clause) valid here of is it better to try to create the session again?

enableKeepAlive was mentioned in conversation with @jburel but I shied away from it because some of our tests don't clean up that well. Maybe this is something for a card when we do some test refactoring - as I am sure we will do.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is something for a card when we do some test refactoring - as I am sure we will do.

👍 -- or if this doesn't work 😄

@ximenesuk ximenesuk closed this Apr 28, 2016
@ximenesuk ximenesuk reopened this Apr 28, 2016
@jburel
Copy link
Copy Markdown
Member

jburel commented Apr 28, 2016

Good to have it opened so we can hopefully have the tests green.

@jburel
Copy link
Copy Markdown
Member

jburel commented Apr 28, 2016

Tested from home against eel.

test/integration/clitest/test_import.py::TestImport::testImportAsRoot PASSED
test/integration/clitest/test_import.py::TestImport::testImportMultiGroup PASSED
test/integration/clitest/test_import.py::TestImport::testImportAsRootMultiGroup PASSED

The same tests always fail w/o the change

@jburel
Copy link
Copy Markdown
Member

jburel commented Apr 29, 2016

the "importAs" tests are passing this morning https://ci.openmicroscopy.org/view/OMERO-DEV/job/OMERO-DEV-merge-integration-python/262/

@joshmoore
Copy link
Copy Markdown
Member

And the masses rejoice!

@jburel
Copy link
Copy Markdown
Member

jburel commented May 2, 2016

Last few builds passed with the change.
Merging thanks @ximenesuk

@jburel jburel merged commit 884f5de into ome:develop May 2, 2016
@jburel jburel added this to the 5.2.3 milestone May 4, 2016
@sbesson sbesson modified the milestones: 5.3.0, 5.2.3 May 10, 2016
@sbesson sbesson modified the milestones: 5.3.0, 5.2.3 Sep 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants