From af070453aa8af0274e28b122ccffe173c1d8e8f1 Mon Sep 17 00:00:00 2001 From: Patrick Peglar Date: Mon, 20 Jun 2022 12:35:15 +0100 Subject: [PATCH] Ensure tests use ncgen/ncdump from the Python env; rationalise uses of ncgen. (#4794) * Ensure tests use ncgen/ncdump from the Python env; rationalise uses of ncgen a bit. * Tidy some imports. * Rationalise use of ncgen and avoid subprocess 'shell=True'. * Review changes: revert to importing iris.tests first. * Tidy routines slightly. * Fix subprocess call. * Added whatsnew. --- docs/src/whatsnew/latest.rst | 6 ++ lib/iris/tests/__init__.py | 34 +++++++++++- .../experimental/test_ugrid_save.py | 24 ++++---- .../tests/integration/test_climatology.py | 17 +++--- lib/iris/tests/integration/test_netcdf.py | 14 ++--- lib/iris/tests/stock/netcdf.py | 55 +++++++++++++++++-- lib/iris/tests/test_netcdf.py | 26 +++------ .../ugrid/load/test_load_meshes.py | 14 ++--- .../nc_load_rules/actions/__init__.py | 13 +---- .../fileformats/netcdf/test_load_cubes.py | 17 ++---- 10 files changed, 134 insertions(+), 86 deletions(-) diff --git a/docs/src/whatsnew/latest.rst b/docs/src/whatsnew/latest.rst index b288c739a7..86476a357f 100644 --- a/docs/src/whatsnew/latest.rst +++ b/docs/src/whatsnew/latest.rst @@ -216,6 +216,12 @@ This document explains the changes made to Iris for this release #. `@bjlittle`_ migrated to GitHub Actions for Continuous-Integration. (:pull:`4503`) +#. `@pp-mo`_ made tests run certain linux executables from the Python env, + specifically ncdump and ncgen. These could otherwise fail when run in IDEs + such as PyCharm and Eclipse, which don't automatically include the Python env + bin in the system PATH. + (:pull:`4794`) + .. comment Whatsnew author names (@github name) in alphabetical order. Note that, diff --git a/lib/iris/tests/__init__.py b/lib/iris/tests/__init__.py index bf8ad7c81d..2cefae505c 100644 --- a/lib/iris/tests/__init__.py +++ b/lib/iris/tests/__init__.py @@ -34,12 +34,13 @@ import math import os import os.path +from pathlib import Path import re import shutil import subprocess import sys import threading -from typing import Dict, List +from typing import AnyStr, Dict, List import unittest from unittest import mock import warnings @@ -373,8 +374,8 @@ def assertCDL(self, netcdf_filename, reference_filename=None, flags="-h"): flags = list(map(str, flags)) try: - # Python3 only: use subprocess.run() - args = ["ncdump"] + flags + [netcdf_filename] + exe_path = env_bin_path("ncdump") + args = [exe_path] + flags + [netcdf_filename] cdl = subprocess.check_output(args, stderr=subprocess.STDOUT) except subprocess.CalledProcessError as exc: print(exc.output) @@ -1348,3 +1349,30 @@ def wrapped(self, *args, **kwargs): return result return wrapped + + +def env_bin_path(exe_name: AnyStr = None): + """ + Return a Path object for (an executable in) the environment bin directory. + + Parameters + ---------- + exe_name : str + If set, the name of an executable to append to the path. + + Returns + ------- + exe_path : Path + A path to the bin directory, or an executable file within it. + + Notes + ----- + For use in tests which spawn commands which should call executables within + the Python environment, since many IDEs (Eclipse, PyCharm) don't + automatically include this location in $PATH (as opposed to $PYTHONPATH). + """ + exe_path = Path(os.__file__) + exe_path = (exe_path / "../../../bin").resolve() + if exe_name is not None: + exe_path = exe_path / exe_name + return exe_path diff --git a/lib/iris/tests/integration/experimental/test_ugrid_save.py b/lib/iris/tests/integration/experimental/test_ugrid_save.py index eb2cb04f79..803ac71caa 100644 --- a/lib/iris/tests/integration/experimental/test_ugrid_save.py +++ b/lib/iris/tests/integration/experimental/test_ugrid_save.py @@ -14,17 +14,15 @@ import glob from pathlib import Path import shutil -from subprocess import check_call import tempfile import iris from iris.experimental.ugrid.load import PARSE_UGRID_ON_LOAD import iris.fileformats.netcdf -from iris.tests import IrisTest -from iris.tests.stock.netcdf import _add_standard_data +from iris.tests.stock.netcdf import _add_standard_data, ncgen_from_cdl -class TestBasicSave(IrisTest): +class TestBasicSave(tests.IrisTest): @classmethod def setUpClass(cls): cls.temp_dir = Path(tempfile.mkdtemp()) @@ -46,11 +44,11 @@ def tearDownClass(cls): def test_example_result_cdls(self): # Snapshot the result of saving the example cases. - for ex_name, filepath in self.example_names_paths.items(): + for ex_name, cdl_path in self.example_names_paths.items(): + # Create a test netcdf file. target_ncfile_path = str(self.temp_dir / f"{ex_name}.nc") - # Create a netcdf file from the test CDL. - check_call( - f"ncgen {filepath} -k4 -o {target_ncfile_path}", shell=True + ncgen_from_cdl( + cdl_str=None, cdl_path=cdl_path, nc_path=target_ncfile_path ) # Fill in blank data-variables. _add_standard_data(target_ncfile_path) @@ -64,18 +62,18 @@ def test_example_result_cdls(self): refdir_relpath = ( "integration/experimental/ugrid_save/TestBasicSave/" ) - reffile_name = str(Path(filepath).name).replace(".nc", ".cdl") + reffile_name = str(Path(cdl_path).name).replace(".nc", ".cdl") reffile_path = refdir_relpath + reffile_name self.assertCDL(resave_ncfile_path, reference_filename=reffile_path) def test_example_roundtrips(self): # Check that save-and-loadback leaves Iris data unchanged, # for data derived from each UGRID example CDL. - for ex_name, filepath in self.example_names_paths.items(): + for ex_name, cdl_path in self.example_names_paths.items(): + # Create a test netcdf file. target_ncfile_path = str(self.temp_dir / f"{ex_name}.nc") - # Create a netcdf file from the test CDL. - check_call( - f"ncgen {filepath} -k4 -o {target_ncfile_path}", shell=True + ncgen_from_cdl( + cdl_str=None, cdl_path=cdl_path, nc_path=target_ncfile_path ) # Fill in blank data-variables. _add_standard_data(target_ncfile_path) diff --git a/lib/iris/tests/integration/test_climatology.py b/lib/iris/tests/integration/test_climatology.py index ba1ccaf888..54d43858fb 100644 --- a/lib/iris/tests/integration/test_climatology.py +++ b/lib/iris/tests/integration/test_climatology.py @@ -13,14 +13,14 @@ from os.path import join as path_join from os.path import sep as os_sep import shutil -from subprocess import check_call import tempfile import iris from iris.tests import stock +from iris.tests.stock.netcdf import ncgen_from_cdl -class TestClimatology(iris.tests.IrisTest): +class TestClimatology(tests.IrisTest): reference_cdl_path = os_sep.join( [ dirname(tests.__file__), @@ -58,12 +58,13 @@ def setUpClass(cls): cls.temp_dir = tempfile.mkdtemp() cls.path_ref_cdl = path_join(cls.temp_dir, "standard.cdl") cls.path_ref_nc = path_join(cls.temp_dir, "standard.nc") - # Create reference CDL file. - with open(cls.path_ref_cdl, "w") as f_out: - f_out.write(cls._simple_cdl_string()) - # Create reference netCDF file from reference CDL. - command = "ncgen -o {} {}".format(cls.path_ref_nc, cls.path_ref_cdl) - check_call(command, shell=True) + # Create reference CDL and netcdf files (with ncgen). + ncgen_from_cdl( + cdl_str=cls._simple_cdl_string(), + cdl_path=cls.path_ref_cdl, + nc_path=cls.path_ref_nc, + ) + cls.path_temp_nc = path_join(cls.temp_dir, "tmp.nc") # Create reference cube. diff --git a/lib/iris/tests/integration/test_netcdf.py b/lib/iris/tests/integration/test_netcdf.py index 4989ef9b6c..ca8c4c7697 100644 --- a/lib/iris/tests/integration/test_netcdf.py +++ b/lib/iris/tests/integration/test_netcdf.py @@ -14,7 +14,6 @@ import os.path from os.path import join as path_join import shutil -from subprocess import check_call import tempfile from unittest import mock import warnings @@ -33,6 +32,7 @@ UnknownCellMethodWarning, ) import iris.tests.stock as stock +from iris.tests.stock.netcdf import ncgen_from_cdl import iris.tests.unit.fileformats.netcdf.test_load_cubes as tlc @@ -864,12 +864,12 @@ def setUpClass(cls): cls.temp_dir = tempfile.mkdtemp() cls.path_test_cdl = path_join(cls.temp_dir, "geos_problem.cdl") cls.path_test_nc = path_join(cls.temp_dir, "geos_problem.nc") - # Create a reference file from the CDL text. - with open(cls.path_test_cdl, "w") as f_out: - f_out.write(cls._geostationary_problem_cdl) - # Call 'ncgen' to make an actual netCDF file from the CDL. - command = "ncgen -o {} {}".format(cls.path_test_nc, cls.path_test_cdl) - check_call(command, shell=True) + # Create reference CDL and netcdf files from the CDL text. + ncgen_from_cdl( + cdl_str=cls._geostationary_problem_cdl, + cdl_path=cls.path_test_cdl, + nc_path=cls.path_test_nc, + ) @classmethod def tearDownClass(cls): diff --git a/lib/iris/tests/stock/netcdf.py b/lib/iris/tests/stock/netcdf.py index 4e12850ef1..3e95fce95e 100644 --- a/lib/iris/tests/stock/netcdf.py +++ b/lib/iris/tests/stock/netcdf.py @@ -8,12 +8,60 @@ from pathlib import Path from string import Template import subprocess +from typing import Optional import dask from dask import array as da import netCDF4 import numpy as np +from iris.tests import env_bin_path + +NCGEN_PATHSTR = str(env_bin_path("ncgen")) + + +def ncgen_from_cdl( + cdl_str: Optional[str], cdl_path: Optional[str], nc_path: str +): + """ + Generate a test netcdf file from cdl. + + Source is CDL in either a string or a file. + If given a string, will either save a CDL file, or pass text directly. + A netcdf output file is always created, at the given path. + + Parameters + ---------- + cdl_str : str or None + String containing a CDL description of a netcdf file. + If None, 'cdl_path' must be an existing file. + cdl_path : str or None + Path of temporary text file where cdl_str is written. + If None, 'cdl_str' must be given, and is piped direct to ncgen. + nc_path : str + Path of temporary netcdf file where converted result is put. + + Notes + ----- + For legacy reasons, the path args are 'str's not 'Path's. + + """ + if cdl_str and cdl_path: + with open(cdl_path, "w") as f_out: + f_out.write(cdl_str) + if cdl_path: + # Create netcdf from stored CDL file. + call_args = [NCGEN_PATHSTR, cdl_path, "-k4", "-o", nc_path] + call_kwargs = {} + else: + # No CDL file : pipe 'cdl_str' directly into the ncgen program. + if not cdl_str: + raise ValueError("Must provide either 'cdl_str' or 'cdl_path'.") + call_args = [NCGEN_PATHSTR, "-k4", "-o", nc_path] + call_kwargs = dict(input=cdl_str, encoding="ascii") + + subprocess.run(call_args, check=True, **call_kwargs) + def _file_from_cdl_template( temp_file_dir, dataset_name, dataset_type, template_subs @@ -39,12 +87,7 @@ def _file_from_cdl_template( # Spawn an "ncgen" command to create an actual NetCDF file from the # CDL string. - subprocess.run( - ["ncgen", "-o" + str(nc_write_path)], - input=cdl, - encoding="ascii", - check=True, - ) + ncgen_from_cdl(cdl_str=cdl, cdl_path=None, nc_path=nc_write_path) return nc_write_path diff --git a/lib/iris/tests/test_netcdf.py b/lib/iris/tests/test_netcdf.py index 5029f822e7..5017698a22 100644 --- a/lib/iris/tests/test_netcdf.py +++ b/lib/iris/tests/test_netcdf.py @@ -16,7 +16,6 @@ import os.path import shutil import stat -from subprocess import check_call import tempfile from unittest import mock @@ -33,6 +32,7 @@ from iris.fileformats.netcdf import load_cubes as nc_load_cubes import iris.std_names import iris.tests.stock as stock +from iris.tests.stock.netcdf import ncgen_from_cdl import iris.util @@ -363,12 +363,8 @@ def test_um_stash_source(self): self.tmpdir = tempfile.mkdtemp() cdl_path = os.path.join(self.tmpdir, "tst.cdl") nc_path = os.path.join(self.tmpdir, "tst.nc") - # Write CDL string into a temporary CDL file. - with open(cdl_path, "w") as f_out: - f_out.write(ref_cdl) - # Use ncgen to convert this into an actual (temporary) netCDF file. - command = "ncgen -o {} {}".format(nc_path, cdl_path) - check_call(command, shell=True) + # Create a temporary netcdf file from the CDL string. + ncgen_from_cdl(ref_cdl, cdl_path, nc_path) # Load with iris.fileformats.netcdf.load_cubes, and check expected content. cubes = list(nc_load_cubes(nc_path)) self.assertEqual(len(cubes), 1) @@ -412,12 +408,8 @@ def test_ukmo__um_stash_source_priority(self): self.tmpdir = tempfile.mkdtemp() cdl_path = os.path.join(self.tmpdir, "tst.cdl") nc_path = os.path.join(self.tmpdir, "tst.nc") - # Write CDL string into a temporary CDL file. - with open(cdl_path, "w") as f_out: - f_out.write(ref_cdl) - # Use ncgen to convert this into an actual (temporary) netCDF file. - command = "ncgen -o {} {}".format(nc_path, cdl_path) - check_call(command, shell=True) + # Create a temporary netcdf file from the CDL string. + ncgen_from_cdl(ref_cdl, cdl_path, nc_path) # Load with iris.fileformats.netcdf.load_cubes, and check expected content. cubes = list(nc_load_cubes(nc_path)) self.assertEqual(len(cubes), 1) @@ -457,12 +449,8 @@ def test_bad_um_stash_source(self): self.tmpdir = tempfile.mkdtemp() cdl_path = os.path.join(self.tmpdir, "tst.cdl") nc_path = os.path.join(self.tmpdir, "tst.nc") - # Write CDL string into a temporary CDL file. - with open(cdl_path, "w") as f_out: - f_out.write(ref_cdl) - # Use ncgen to convert this into an actual (temporary) netCDF file. - command = "ncgen -o {} {}".format(nc_path, cdl_path) - check_call(command, shell=True) + # Create a temporary netcdf file from the CDL string. + ncgen_from_cdl(ref_cdl, cdl_path, nc_path) # Load with iris.fileformats.netcdf.load_cubes, and check expected content. cubes = list(nc_load_cubes(nc_path)) self.assertEqual(len(cubes), 1) diff --git a/lib/iris/tests/unit/experimental/ugrid/load/test_load_meshes.py b/lib/iris/tests/unit/experimental/ugrid/load/test_load_meshes.py index f2175ef99a..310e68248a 100644 --- a/lib/iris/tests/unit/experimental/ugrid/load/test_load_meshes.py +++ b/lib/iris/tests/unit/experimental/ugrid/load/test_load_meshes.py @@ -13,7 +13,6 @@ from pathlib import Path from shutil import rmtree -from subprocess import check_call import tempfile from uuid import uuid4 @@ -22,6 +21,7 @@ load_meshes, logger, ) +from iris.tests.stock.netcdf import ncgen_from_cdl def setUpModule(): @@ -35,15 +35,11 @@ def tearDownModule(): def cdl_to_nc(cdl): - cdl_path = TMP_DIR / "tst.cdl" - nc_path = TMP_DIR / f"{uuid4()}.nc" - # Write CDL string into a temporary CDL file. - with open(cdl_path, "w") as f_out: - f_out.write(cdl) + cdl_path = str(TMP_DIR / "tst.cdl") + nc_path = str(TMP_DIR / f"{uuid4()}.nc") # Use ncgen to convert this into an actual (temporary) netCDF file. - command = "ncgen -o {} {}".format(nc_path, cdl_path) - check_call(command, shell=True) - return str(nc_path) + ncgen_from_cdl(cdl_str=cdl, cdl_path=cdl_path, nc_path=nc_path) + return nc_path class TestsBasic(tests.IrisTest): diff --git a/lib/iris/tests/unit/fileformats/nc_load_rules/actions/__init__.py b/lib/iris/tests/unit/fileformats/nc_load_rules/actions/__init__.py index 2f627f0a2c..7bcb451d95 100644 --- a/lib/iris/tests/unit/fileformats/nc_load_rules/actions/__init__.py +++ b/lib/iris/tests/unit/fileformats/nc_load_rules/actions/__init__.py @@ -9,7 +9,6 @@ """ from pathlib import Path import shutil -import subprocess import tempfile import warnings @@ -17,6 +16,7 @@ from iris.fileformats.cf import CFReader import iris.fileformats.netcdf from iris.fileformats.netcdf import _load_cube +from iris.tests.stock.netcdf import ncgen_from_cdl """ Notes on testing method. @@ -30,9 +30,7 @@ As it's hard to construct a suitable CFReader from scratch, it would seem simpler (for now) to use an ACTUAL FILE. Likewise, the easiest approach to that is with CDL and "ncgen". -To do this, we need a test "fixture" that can create suitable test files in a -temporary directory. - +For this, we just use 'tests.stock.netcdf.ncgen_from_cdl'. """ @@ -78,12 +76,7 @@ def load_cube_from_cdl(self, cdl_string, cdl_path, nc_path): """ # Write the CDL to a file. - with open(cdl_path, "w") as f_out: - f_out.write(cdl_string) - - # Create a netCDF file from the CDL file. - command = "ncgen -o {} {}".format(nc_path, cdl_path) - subprocess.check_call(command, shell=True) + ncgen_from_cdl(cdl_string, cdl_path, nc_path) # Simulate the inner part of the file reading process. cf = CFReader(nc_path) diff --git a/lib/iris/tests/unit/fileformats/netcdf/test_load_cubes.py b/lib/iris/tests/unit/fileformats/netcdf/test_load_cubes.py index bbcf2cc72b..39992d03a0 100644 --- a/lib/iris/tests/unit/fileformats/netcdf/test_load_cubes.py +++ b/lib/iris/tests/unit/fileformats/netcdf/test_load_cubes.py @@ -11,9 +11,12 @@ """ +# Import iris.tests first so that some things can be initialised before +# importing anything else. +import iris.tests as tests # isort:skip + from pathlib import Path from shutil import rmtree -from subprocess import check_call import tempfile from cf_units import as_unit @@ -23,10 +26,7 @@ from iris.experimental.ugrid.load import PARSE_UGRID_ON_LOAD from iris.experimental.ugrid.mesh import MeshCoord from iris.fileformats.netcdf import load_cubes, logger - -# Import iris.tests first so that some things can be initialised before -# importing anything else. -import iris.tests as tests +from iris.tests.stock.netcdf import ncgen_from_cdl def setUpModule(): @@ -42,12 +42,7 @@ def tearDownModule(): def cdl_to_nc(cdl): cdl_path = TMP_DIR / "tst.cdl" nc_path = TMP_DIR / "tst.nc" - # Write CDL string into a temporary CDL file. - with open(cdl_path, "w") as f_out: - f_out.write(cdl) - # Use ncgen to convert this into an actual (temporary) netCDF file. - command = "ncgen -o {} {}".format(nc_path, cdl_path) - check_call(command, shell=True) + ncgen_from_cdl(cdl, cdl_path, nc_path) return str(nc_path)