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

Gateway methods #5545

Merged
merged 11 commits into from
Nov 23, 2017
Merged

Gateway methods #5545

merged 11 commits into from
Nov 23, 2017

Conversation

jburel
Copy link
Member

@jburel jburel commented Oct 13, 2017

What this PR does

  • Port commits currently in metadata 53. The Commits were used by the cli plugin render
  • Add tests for new methodss
  • Introduce set_active_channels and deprecate setActiveChannels
  • Review tests and make sure that all services are closed

Related reading

see ome/omero-cli-render#1

To test

  • Check that the tests are green

cc @joshmoore

A new "tracked service" internal API has been added. All calls to
`_create_func` in `ProxyObjectWrapper` now registers the string
representation of the created service as well as the current call
stack to the owning `BlitzGateway` instance. If on deletion, services
are left open, then an ERROR message including the call stack will
be printed.
@jburel jburel added the develop label Oct 13, 2017
@dominikl
Copy link
Member

dominikl commented Oct 16, 2017

Tests are passing and make sense. Given that really all services are registered via the_register_service method. I can't really work it out how that works. Would it be possible to add a test for that, too? Because in case they wouldn't get registered in first place, assert not g._assert_unregistered(...) in the tests would always pass.

@jburel
Copy link
Member Author

jburel commented Oct 16, 2017

All services are registered, what is important is to make sure that they are all closed.
This was not the case before the changes i.e. 17 services were left opened
This is why assert not g._assert_unregistered has been added at the end of all the tests

@joshmoore
Copy link
Member

A bit concerned by how invasive some of the changes are (i.e. it will be easy to forget to follow the patterns when writing new tests). 2 questions:

  • Can the test assertion be done at a higher level? e.g. during tear down. We track all omero.client creations; we could do the same for gateways.
  • Are the remaining open services indicative of a code problem as opposed to a test problem? Does this mean that OMERO.web is also leaving them open?

@jburel
Copy link
Member Author

jburel commented Oct 24, 2017

This will require discussion since I do not know what was the initial plan on the metadata branch
The strategy in the tests is the one used in the metadata branch for render i.e. check at the end of each test
It certainly helps tracking down open service in the tests.
OMERO.web is probably leaving a good number of service open but I think it is mainly a test problem. I did not review that as part of this PR.
The main goal is to make the structure accessible to the plugins

@joshmoore joshmoore changed the base branch from develop to clisplit November 21, 2017 10:45
@joshmoore
Copy link
Member

This has been in for some time. Merging to take a look at the omero-cli-render repo. Note: created https://trello.com/c/Qc6QEQS0/40-remove-need-for-imgclosere to come back to the changes here and try to remove the invasiveness.

@joshmoore joshmoore merged commit b476a51 into ome:clisplit Nov 23, 2017
@joshmoore joshmoore added this to the 5.4.2 milestone Jan 16, 2018
@jburel jburel deleted the gateway-methods branch September 25, 2018 13:47
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.

3 participants