Delete unnecessary test and context manager

[ntBre]

Description

As discussed in #286, the no_internet context manager added in #284 does not test what I thought it tested (that existing PortalClient instances cannot access QCArchive over the internet). The _CachedPortalClient._no_session context manager is a better way to guarantee this, and the usage of portal_client_manager in other tests also renders the test_manager test itself redundant. As a result, the whole test_manager.py file can be deleted. I also updated the _no_session docs to avoid referring to the now-deleted no_internet manager.

Status

• Ready to go

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.43%. Comparing base (357e2de) to head (002e612).
Report is 8 commits behind head on main.

Additional details and impacted files

[ntBre]

Coverage actually decreases by 0.02% here because this code is uncovered:

def _default_portal_client(client_address) -> PortalClient:
     return PortalClient(client_address) # this line

I think it might be a good idea to modify at least one of the tests using portal_client_manager to ensure that a plain PortalClient works. I changed every test to use _CachedPortalClient when it was going to become the new default in #286 to avoid sharing the same cache directories, but now it should be safe to get rid of some of those TemporaryDirectory and portal_client_manager calls again.

[ntBre]

I'm a bit suspicious of the last test failure. I don't think any of my changes should have caused this, at least. It seems like it could be a rare timing failure that we happen not to have hit yet, but I'm not sure. The RecordStatusEnum variants haven't changed since 2022 according to git blame. In any case, I made await_results check all 7 variants instead of the looser check it had before and raise an error if a new one is encountered.

Anyway, codecov is now reporting a 0.00% change from main, as desired!

[j-wags]

Looks good, thanks for coming back to smooth this over. Please update the releasenotes before merging!

[j-wags]

(not blocking)

I think it might be a good idea to modify at least one of the tests using portal_client_manager to ensure that a plain PortalClient works. I changed every test to use _CachedPortalClient when it was going to become the new default in #286 to avoid sharing the same cache directories, but now it should be safe to get rid of some of those TemporaryDirectory and portal_client_manager calls again.

Yeah, it's good that you caught that default_portal_client was going to go uncovered and changed some of these back. It seems a little brittle that a newcomer wouldn't understand why some tests are using portal_client_manager and others aren't. But that can be a topic for a future PR (maybe a dedicated test for default/non-default clients? Or some pattern of using pytest.parameterize that doesn't double runtime?). If you can think of a clean comment to leave explaining this (like # this test doesn't use portal_client_manager just so it can test out the default pathway) could you add it before merging?

[ntBre]

Should I include this comment in each of the four tests that no longer use portal_client_manager? I guess I was thinking that since that is the default behavior, it makes sense for it to be the default test behavior too. Possibly I should instead comment on the one test still using portal_client_manager (test_to_records) to point out why it's being used there?

Maybe it would have been better if I had just copy-pasted test_to_records to begin with to have a dedicated test_cached_to_records test as you mention. I thought I would only have to make small changes to test_to_records at first, but in the end it basically became a different test anyway.

[j-wags]

Let's put a comment on the one test that uses caching, like "as of Aug 2024, this is the one place we're testing caching"

[ntBre]

Please update the releasenotes before merging!

I think I only changed tests and a docstring in a private method. Should I still add something to the releasenotes?

[j-wags]

Ah, good catch, no need to update releasenotes.