-
Notifications
You must be signed in to change notification settings - Fork 25
Update system caches tests #697
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
Conversation
|
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
@@ -131,19 +137,6 @@ def test_system_caches_delete_admin(self): | |||
|
|||
# TODO: Unsure how to test log messages received from Connect. | |||
|
|||
# Admins cannot delete caches that do not exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should not be here imho,
it's testing a connect behaviour not rsconnect-python
.
We are already testing that deleting a cache does work, and we are already testing that reporting an error message from delete works:
test_system_caches_delete_admin
test_system_caches_delete_publisher
So this test didn't test anything additional and made the testsuite too dependant on a connect behaviours that didn't break compatibility with rsconnect itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test-only fix? There's nothing in the code that needs to be adapted to point at the new cache location?
|
||
ADD_CACHE_COMMAND = "docker compose exec -u rstudio-connect -T rsconnect mkdir -p /data/python-environments/pip/1.2.3" | ||
RM_CACHE_COMMAND = "docker compose exec -u rstudio-connect -T rsconnect rm -Rf /data/python-environments/pip/1.2.3" | ||
ADD_CACHE_COMMAND = f"docker compose exec -u rstudio-connect -T rsconnect mkdir -p {CONNECT_CACHE_DIR}/pip/1.2.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests that shell out to docker compose
? We don't have to change that here, but that doesn't feel right, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, there are a few Connect implementation details that are involved in rsconnect-python
tests.
While Integration tests make sense, I believe they should treat connect as a blackbox.
For example, as far as "delete + list again" API calls confirm that the cache was actually deleted, we should not care if that happened for real or not on disk. Worst case, that would be a Connect bug and not a rsconnect one, and would be caught by connect tests. Rsconnect should start from the assumption that Connect does the right thing.
Yes, test only change as tests were failing on main |
Intent
connect path to system caches has moved inside
python-environments/_packages_cache
, the test suite is still looking in the old path.Type of Change