fix error message for XDR readers (#4231)

* fix error message for XDR readers

- error message for not being able to write lockfile did not insert the filename into
  message: fixed by using proper f string
- changed all .format to modern f string in XDR.py
- add explicit test for offsets_filename()
- add additional pytest.warns() to tests to catch warnings (reduces warnings output and
  checks that all expected warnings are raised with proper messages)

* more robust checking of multiple warnings

* Apply suggestions from code review

Co-authored-by: Irfan Alibay <[email protected]>

* simplified testing for multiple warnings

* more explicit warnings testing

- reduces warnings in test_xdr down to 2 (both related to del)
- test for empty selection and missing elements warnings
- rewriting more old-school parameter interpolation to f strings

* only use f-strings where used

Co-authored-by: Rocco Meli <[email protected]>

* ignore warnings not germane to the XDR tests

---------

Co-authored-by: Irfan Alibay <[email protected]>
Co-authored-by: Rocco Meli <[email protected]>

package/MDAnalysis/coordinates/XDR.py

@@ -63,8 +63,7 @@ def offsets_filename(filename, ending='npz'):
 
     """
     head, tail = split(filename)
-    return join(head, '.{tail}_offsets.{ending}'.format(tail=tail,
-                                                        ending=ending))
+    return join(head, f'.{tail}_offsets.{ending}')
 
 
 def read_numpy_offsets(filename):
@@ -88,7 +87,7 @@ def read_numpy_offsets(filename):
 
     #  `ValueError` is encountered when the offset file is corrupted.
     except (ValueError, IOError):
-        warnings.warn("Failed to load offsets file {}\n".format(filename))
+        warnings.warn(f"Failed to load offsets file {filename}\n")
         return False
 
 class XDRBaseReader(base.ReaderBase):
@@ -201,7 +200,7 @@ def _load_offsets(self):
         except OSError as e:
             if isinstance(e, PermissionError) or e.errno == errno.EROFS:
                 warnings.warn(f"Cannot write lock/offset file in same location as "
-                               "{self.filename}. Using slow offset calculation.")
+                              f"{self.filename}. Using slow offset calculation.")
                 self._read_offsets(store=True)
                 return
             else:
@@ -220,10 +219,10 @@ def _load_offsets(self):
             # refer to Issue #1893
             data = read_numpy_offsets(fname)
             if not data:
-                warnings.warn("Reading offsets from {} failed, "
+                warnings.warn(f"Reading offsets from {fname} failed, "
                               "reading offsets from trajectory instead.\n"
                               "Consider setting 'refresh_offsets=True' "
-                              "when loading your Universe.".format(fname))
+                              "when loading your Universe.")
                 self._read_offsets(store=True)
                 return
 
@@ -256,7 +255,7 @@ def _read_offsets(self, store=False):
                          offsets=offsets, size=size, ctime=ctime,
                          n_atoms=self._xdr.n_atoms)
             except Exception as e:
-                warnings.warn("Couldn't save offsets because: {}".format(e))
+                warnings.warn(f"Couldn't save offsets because: {e}")
 
     @property
     def n_frames(self):

testsuite/MDAnalysisTests/coordinates/base.py

@@ -443,13 +443,13 @@ def test_pickle_reader(self, reader):
         assert_equal(len(reader), len(reader_p))
         assert_equal(reader.ts, reader_p.ts,
                      "Timestep is changed after pickling")
-    
+
     def test_frame_collect_all_same(self, reader):
-        # check that the timestep resets so that the base reference is the same 
+        # check that the timestep resets so that the base reference is the same
         # for all timesteps in a collection with the exception of memoryreader
         # and DCDReader
         if isinstance(reader, mda.coordinates.memory.MemoryReader):
-            pytest.xfail("memoryreader allows independent coordinates") 
+            pytest.xfail("memoryreader allows independent coordinates")
         if isinstance(reader, mda.coordinates.DCD.DCDReader):
             pytest.xfail("DCDReader allows independent coordinates."
                           "This behaviour is deprecated and will be changed"
@@ -493,9 +493,11 @@ def test_timeseries_asel_shape(self, reader, asel):
         assert(timeseries.shape[0] == len(reader))
         assert(timeseries.shape[1] == len(atoms))
         assert(timeseries.shape[2] == 3)
-    
+
     def test_timeseries_empty_asel(self, reader):
-        atoms = mda.Universe(reader.filename).select_atoms(None)
+        with pytest.warns(UserWarning,
+                          match="Empty string to select atoms, empty group returned."):
+            atoms = mda.Universe(reader.filename).select_atoms(None)
         with pytest.raises(ValueError, match="Timeseries requires at least"):
             reader.timeseries(atoms)
 

testsuite/MDAnalysisTests/coordinates/test_xdr.py

@@ -23,6 +23,7 @@
 import pytest
 from unittest.mock import patch
 
+import re
 import os
 import shutil
 import subprocess
@@ -47,6 +48,16 @@
 from MDAnalysisTests.util import get_userid
 
 
+@pytest.mark.parametrize("filename,kwargs,reference", [
+    ("foo.xtc", {}, ".foo.xtc_offsets.npz"),
+    ("foo.xtc", {"ending": "npz"}, ".foo.xtc_offsets.npz"),
+    ("bar.0001.trr", {"ending": "npzzzz"}, ".bar.0001.trr_offsets.npzzzz"),
+])
+def test_offsets_filename(filename, kwargs, reference):
+    fn = XDR.offsets_filename(filename, **kwargs)
+    assert fn == reference
+
+
 class _XDRReader_Sub(object):
     @pytest.fixture()
     def atoms(self):
@@ -212,7 +223,7 @@ def go_beyond_EOF():
 
         with pytest.raises(StopIteration):
             go_beyond_EOF()
-    
+
     def test_read_next_timestep_ts_no_positions(self, universe):
         # primarily tests branching on ts.has_positions in _read_next_timestep
         ts = universe.trajectory[0]
@@ -544,6 +555,7 @@ class _GromacsWriterIssue117(object):
     def universe(self):
         return mda.Universe(PRMncdf, NCDF)
 
+    @pytest.mark.filterwarnings("ignore: ATOMIC_NUMBER record not found")
     def test_write_trajectory(self, universe, tmpdir):
         """Test writing Gromacs trajectories from AMBER NCDF (Issue 117)"""
         outfile = str(tmpdir.join('xdr-writer-issue117' + self.ext))
@@ -559,10 +571,9 @@ def test_write_trajectory(self, universe, tmpdir):
                 written_ts._pos,
                 orig_ts._pos,
                 self.prec,
-                err_msg="coordinate mismatch "
-                "between original and written "
-                "trajectory at frame %d (orig) vs %d "
-                "(written)" % (orig_ts.frame, written_ts.frame))
+                err_msg=("coordinate mismatch between original and written "
+                         f"trajectory at frame {orig_ts.frame:d} (orig) vs "
+                         f"{orig_ts.frame:d} (written)"))
 
 
 class TestXTCWriterIssue117(_GromacsWriterIssue117):
@@ -755,25 +766,34 @@ def test_nonexistent_offsets_file(self, traj):
         outfile_offsets = XDR.offsets_filename(traj)
         with patch.object(np, "load") as np_load_mock:
             np_load_mock.side_effect = IOError
-            saved_offsets = XDR.read_numpy_offsets(outfile_offsets)
-            assert_equal(saved_offsets, False)
+            with pytest.warns(UserWarning, match=re.escape(
+                    f"Failed to load offsets file {outfile_offsets}")):
+                saved_offsets = XDR.read_numpy_offsets(outfile_offsets)
+        assert saved_offsets == False
 
-    def test_nonexistent_offsets_file(self, traj):
+    def test_corrupted_offsets_file(self, traj):
         # assert that a corrupted file returns False during read-in
         # Issue #3230
         outfile_offsets = XDR.offsets_filename(traj)
         with patch.object(np, "load") as np_load_mock:
             np_load_mock.side_effect = ValueError
-            saved_offsets = XDR.read_numpy_offsets(outfile_offsets)
-            assert_equal(saved_offsets, False)
+            with pytest.warns(UserWarning, match=re.escape(
+                    f"Failed to load offsets file {outfile_offsets}")):
+                saved_offsets = XDR.read_numpy_offsets(outfile_offsets)
+        assert saved_offsets == False
 
     def test_reload_offsets_if_offsets_readin_io_fails(self, trajectory):
         # force the np.load call that is called in read_numpy_offsets
         # during _load_offsets to give an IOError
         # ensure that offsets are then read-in from the trajectory
         with patch.object(np, "load") as np_load_mock:
             np_load_mock.side_effect = IOError
-            trajectory._load_offsets()
+            with (pytest.warns(UserWarning,
+                               match="Failed to load offsets file") and
+                  pytest.warns(UserWarning,
+                               match="reading offsets from trajectory instead")):
+                trajectory._load_offsets()
+
             assert_almost_equal(
                 trajectory._xdr.offsets,
                 self.ref_offsets,
@@ -866,7 +886,8 @@ def test_unsupported_format(self, traj):
         np.savez(fname, **saved_offsets)
 
         # ok as long as this doesn't throw
-        reader = self._reader(traj)
+        with pytest.warns(UserWarning, match="Reload offsets from trajectory"):
+            reader = self._reader(traj)
         reader[idx_frame]
 
     @pytest.mark.skipif(get_userid() == 0, reason="cannot readonly as root")