openforcefield / openff-qcsubmit

Automated tools for submitting molecules to QCFractal
https://openff-qcsubmit.readthedocs.io/en/latest/index.html
MIT License
26 stars 4 forks source link

Delete unnecessary test and context manager #292

Closed ntBre closed 1 month ago

ntBre commented 1 month ago

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

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.43%. Comparing base (357e2de) to head (002e612).

Additional details and impacted files
ntBre commented 1 month ago

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 commented 1 month ago

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!

ntBre commented 1 month ago

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 commented 1 month ago

Ah, good catch, no need to update releasenotes.