ome / omero-py

Python project containing Ice remoting code for OMERO
https://www.openmicroscopy.org/omero
GNU General Public License v2.0
20 stars 33 forks source link

Assert connection decorator, see #289 #290

Closed glyg closed 2 years ago

glyg commented 3 years ago

@will-moore suggested solving #289 with an assertConnected decorator

IDK how to set up a mock connection to test. I tested with my instance, but the current test wont pass

will-moore commented 3 years ago

Since these are just unit tests, you probably don't want to do all the work needed to mock-up conn.getEventContext()

Instead, you could simply test the decorator itself (simple example below) and if it's also needed to test this for conn.getEventContext() etc then move that to the integration tests at https://github.com/ome/openmicroscopy/tree/develop/components/tools/OmeroPy/test/integration

    class MockGateway():
        _connected = False

        def connect(self):
            self._connected = True

    def foo(conn):
        pass

    wrapped_foo = assertConnected(foo)

    mock_conn = MockGateway()
    with pytest.raises(Ice.ConnectionLostException):
        wrapped_foo(mock_conn)

    mock_conn.connect()
    wrapped_foo(mock_conn)
glyg commented 3 years ago

I put the class def inside the test function as I saw there were other mocks in the file, not sure it is the right thing to do

will-moore commented 3 years ago

Currently failing at assert conn.connect() since that returns None

glyg commented 3 years ago

sorry, forgot to run the test before the push :/

snoopycrimecop commented 3 years ago

Conflicting PR. Removed from build OMERO-python-superbuild-push#712. See the console output for more details. Possible conflicts:

--conflicts Conflict resolved in build OMERO-python-superbuild-push#713. See the console output for more details.

will-moore commented 3 years ago

I've closed #276 which seems to be the PR responsible for the conflict above. Hopefully should be in the merge build now...

joshmoore commented 3 years ago

@will-moore : did you give this a try?

will-moore commented 3 years ago

Yes:

With this PR:

>>> from omero.gateway import BlitzGateway
>>> conn = BlitzGateway("user", "pass", host="host", port=4064)
>>> conn.getEventContext()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/wmoore/Desktop/PY/omero-py/target/omero/gateway/__init__.py", line 1519, in wrapped
    raise Ice.ConnectionLostException("Trying to call %s while not connected" % func.__name__)
Ice.ConnectionLostException: <exception str() failed>

Seems to be working, although Ice.ConnectionLostException: <exception str() failed> doesn't look quite right.

Without this PR:

>>> conn.getEventContext()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/anaconda3/envs/omeroweb/lib/python3.9/site-packages/omero/gateway/__init__.py", line 2322, in getEventContext
    if self._ctx is None:
AttributeError: '_BlitzGateway' object has no attribute '_ctx'
glyg commented 3 years ago
raise Ice.ConnectionLostException("Trying to call %s while not connected" % func.__name__)
Ice.ConnectionLostException: <exception str() failed>

Uh? Maybe using a format string would solve the problem? Trying that now

will-moore commented 3 years ago

I'm afraid that doesn't make a lot of difference for me, but don't know why not!

>>> conn.getEventContext()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/wmoore/Desktop/PY/omero-py/target/omero/gateway/__init__.py", line 1519, in wrapped
    raise Ice.ConnectionLostException(f"Trying to call {func.__name__} while not connected")
Ice.ConnectionLostException: <exception str() failed>
glyg commented 3 years ago

I tested generating the same kind of error message in a generic decorator and did not have the problem (with a ValueError). Maybe something strange going on with IceConnectionLostException? I'm trying without fomatting now