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

Move some unit test helper modules into galaxy-app #12553

Closed

Conversation

jmchilton
Copy link
Member

In the last two commits here, I moved galaxy_mock.py and tools_support.py respectively into a module where they will be packaged with galaxy-app. A precondition to #12549 it seems and another step along the path toward #8364 (along with #12552, #12551, #12550, etc..).

Builds on #12546.

For the other packages I created a unittest_utils in galaxy.<package> - but I'm guessing we shouldn't create a galaxy.app folder I guess since there is a lib/galaxy/app.py file so I just placed these test helpers in app_unittest_utils instead - spiritually follows the convention if not literally.

How to test the changes?

(Select all options that apply)

  • This is a refactoring of components with existing test coverage.

License

@jmchilton jmchilton added kind/refactoring cleanup or refactoring of existing code, no functional changes area/testing labels Sep 26, 2021
@github-actions github-actions bot added this to the 22.01 milestone Sep 26, 2021
@nsoranzo
Copy link
Member

Can you fix the conflict please?

For the other packages I created a unittest_utils in galaxy.<package> - but I'm guessing we shouldn't create a galaxy.app folder I guess since there is a lib/galaxy/app.py file so I just placed these test helpers in app_unittest_utils instead - spiritually follows the convention if not literally.

Mmmh, what about moving lib/galaxy/app.py to lib/galaxy/app/__init__.py instead?

@jmchilton
Copy link
Member Author

Mmmh, what about moving lib/galaxy/app.py to lib/galaxy/app/init.py instead?

I've got a bad history with doing those kind of refactorings - I'm worried about the pyc files conflicting as you switch between 21.09 and 22.01. Are those kind of issues any better with Python 3? I don't think getting the right name is worth the potential deployment issues. Also we do all sort so of gross things with galaxy.app (https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/webapps/galaxy/buildapp.py#L61) right near the top of the app. Am I being too risk adverse and Python ignorant though?

@nsoranzo
Copy link
Member

I did a quick test with a module in BioBlend (back and forth) and it worked fine, but I wouldn't swear to it. Maybe we could try and see if at least the tests pass?

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Sep 28, 2021
This allows for storing more galaxy-app related functionality in a structured place for it - see conversation on galaxyproject#12553.
@jmchilton
Copy link
Member Author

I've opened a PR for the tests you recommended #12583.

I'm still nervous - we've got other stuff that is imported from root by galaxy-app (di.py, queue_worker.py, config_watchers.py, etc..) - what do you think about just renaming galaxy.app_unittest_utils to galaxy.unittest_utils and call it matching the convention rather than #12583.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Sep 28, 2021
This allows for storing more galaxy-app related functionality in a structured place for it - see conversation on galaxyproject#12553.
jmchilton added a commit to jmchilton/galaxy that referenced this pull request Sep 28, 2021
This allows for storing more galaxy-app related functionality in a structured place for it - see conversation on galaxyproject#12553.
@jmchilton jmchilton closed this Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants