MITgcm / xmitgcm

Read MITgcm mds binary files into xarray
http://xmitgcm.readthedocs.io
MIT License
56 stars 65 forks source link

2 Problems with test_mds_store #312

Open timothyas opened 2 years ago

timothyas commented 2 years ago

First off, I think this if statement in test_extra_variables: https://github.com/MITgcm/xmitgcm/blob/63ba7511c6ada3bb7c56e4c6f7a3f770c9f9c62f/xmitgcm/test/test_mds_store.py#L588-L595 causes the test to never actually run. The reasoning is as follows. This test copies the files "U", "V", and "T" to "datatestU/V/T" which the extra_variables capability can handle. The if statement seems to be checking if "U", "V", "T" are in the test data in the first place, and if they are not, skip this test. However, it does this by looking at os.listdir(), which lists the contents of the current working directory not the location of the test data. So, this should be changed to os.listdir(dirname), which is the location of the test data.

If we do this, we arrive at the second and potentially more challenging problem. The test is now doing what it's supposed to, but it fails. However, it doesn't fail because of a problem with extra_variables, it fails because (I think) some mutable object is getting changed somewhere in the test module. My reasoning for this is because we can get the module to pass all tests if we simply put test_extra_variables to be the first test (or at least before test_open_mdsdataset_minimal). Another fix to the problem is by setting the "scope" of the fixture all_mds_datadirs to "function" rather than "module", here: https://github.com/MITgcm/xmitgcm/blob/63ba7511c6ada3bb7c56e4c6f7a3f770c9f9c62f/xmitgcm/test/test_xmitgcm_common.py#L281-L283

Changing the scope of the fixture seems fine... but my concern is that the problem could be arising from somewhere in open_mdsdataset rather than in the test module(s) themselves. I'm not really sure and I'm curious if anyone has any thoughts on this. I'll be looking into it periodically when I can. I have written a very similar test as test_extra_variables but for the custom_grid_variables capability in #308, so I'd rather have this figured out before merging that PR.