Refactored the loop in the tests by parameterizing #736
Conversation
Sherwin-14
changed the title
Sherwin-14
changed the title
| for daac in earthaccess.daac.DAACS: | ||
| if len(daac["s3-credentials"]) > 0: | ||
| try: | ||
| logger.info(f"Testing S3 credentials for {daac['short-name']}") |
There was a problem hiding this comment.
Since the value of daac will be a string, subscript access will throw an error.
Currently integration tests are silently failing, even though they give a green check. It looks like this has been happening for a while. I'm investigating. #737
Once that's fixed, then we can continue to debug this test :)
There was a problem hiding this comment.
Maybe we should wait till then?
There was a problem hiding this comment.
#738 resolves the issue, hopefully we can merge the change soon, but even after that there are errors blocking this test from executing. If you are interested, I think some tests need to be refactored to use fixtures.
There was a problem hiding this comment.
Yeah but which one's need refactoring. If you could provide some info then maybe I can proceed :)
There was a problem hiding this comment.
There are a couple refactors we need:
- Tests which import
assertionsand call for exampleassertions.assertTrue(x)should be updated to builtin assertions, for exampleassert x is True. - Tests which do setup at the module level should be updated to use fixtures
I think these should probably be done separately if you want to take them on; the first one would probably be the best place to start IMO because it will give us better error messages.
This test module demonstrates both things I think we're looking to change: https://github.com/nsidc/earthaccess/blob/main/tests/integration/test_onprem_open.py
Perhaps the best place to start though is to see if we already have issues open for this, and if not, create one. If you can help out with that, it would be awesome :)
There was a problem hiding this comment.
I might create an issue but my descriptions are going to pretty vague( I am not good at explaining at all).
There was a problem hiding this comment.
The team will be happy to help out :) Thanks @Sherwin-14 !
| ("ASDC", "LARC_CLOUD"), | ||
| ), | ||
| ) | ||
| def test_auth_can_fetch_s3_credentials(daac, provider): |
There was a problem hiding this comment.
We don't need provider here. We can also re-use the pre-existing earthaccess.daac.DAACS constant variable for the data structure instead!
There was a problem hiding this comment.
Are you suggesting to use earthaccess.daac.DAACS as the variable from which we can access daac and providers, hence it cuts down the need of having a huge list of parameters ( the ones that I listed above)?
There was a problem hiding this comment.
Correct, and also, the test never uses the provider variable, so it can be removed from the arguments and from the parameters in the decorator :)
I refactored the loop in here. I could only find this one to be refactored. What more could be done here?
馃摎 Documentation preview 馃摎: https://earthaccess--736.org.readthedocs.build/en/736/