Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make ncl or R tests not fail if package not installed #610

Merged
merged 3 commits into from
Apr 20, 2020

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Apr 6, 2020

Before you start, please read CONTRIBUTING.md.

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
  • If writing a new/modified preprocessor function, please update the documentation
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • Please use yamllint to check that your YAML files do not contain mistakes
  • If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Closes {Link to corresponding issue}

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if silently skipping is the right way to go.

If you do not want to run tests that require installation of additional languages, you can skip tests that are marked as installation by running the tests with python setup.py test --addopts '-m "not installation"', as also documented here: https://esmvaltool.readthedocs.io/projects/esmvalcore/en/latest/contributing.html#running-tests

@valeriupredoi
Copy link
Contributor Author

@bouweandela skipping is something we want since the regular Joe will run python setup.py test rather than look up what extra options and how to run in advanced ways the tests. They'll immediately ask 'oh tests failing, guess smth is wrong' - I got asked this a lot of times.

@bouweandela
Copy link
Member

Maybe you're right, but note that if they do have an interpreter installed, but not the required libraries, the tests will still fail.

@bouweandela
Copy link
Member

It would be nicer to use the pytest xfail marker to skip the tests, e.g.

def interpreter_not_installed(script):
    """Check if an interpreter is installed for script."""
    interpreters = {
        '.jl': 'julia',
        '.ncl': 'ncl',
        '.py': 'python',
        '.R': 'Rscript',
    }
    ext = Path(script).suffix
    interpreter = interpreters[ext]
    return which(interpreter) is None


@pytest.mark.parametrize('script_file, script', [
    pytest.param(
        script,
        content,
        marks=[
            pytest.mark.installation,
            pytest.mark.xfail(interpreter_not_installed(script),
                              run=False,
                              reason="Interpreter not available"),
        ],
    ) for script, content in SCRIPTS.items()
])
def test_diagnostic_run(tmp_path, script_file, script):
...

@valeriupredoi
Copy link
Contributor Author

Maybe you're right, but note that if they do have an interpreter installed, but not the required libraries, the tests will still fail.

which is probably the desired behaviour? In any case, I don't think that's true, since if they have an interpreter in the path it means they are using a mixed environment (ie conda path for the esmvaltool environment + other libraries paths) which is a major headache and that'll hit the fan when they run the first R or ncl diagnostic. In fact, we should probably produce a test that fails if ncl and R are not the ones installed in the esmvaltool environment

@bouweandela
Copy link
Member

which is probably the desired behaviour?

Not really, because you can use ESMValCore as a standalone package for running diagnostics that are not part of the ESMValTool package. If you want to use it only with Python diagnostics, there is no need to install R or R dependencies. Yet the test will fail if you happen to have R installed as a system package but not the R yaml library. In that case you will need to use the installation pytest marker to skip the test, because the workaround in this pull request will not skip the test and it will fail.

if they have an interpreter in the path it means they are using a mixed environment

Conda environments are not standalone, they are always mixed with whatever else is installed on your machine

In fact, we should probably produce a test that fails if ncl and R are not the ones installed in the esmvaltool environment

I think this will cause more confusion than be helpful and I think this is a feature we might want to support. If someone already has ncl or R interpreters installed on their machine with whatever libraries they need, I see no point in forcing them into using the packages from conda. For Julia this is not even possible, as it cannot be installed from conda. Once we have ESMValTool on PyPI as well, it might even be possible to use it without conda (though the iris dependencies may not be that easy to install without it).

@valeriupredoi
Copy link
Contributor Author

cheers Bouwe man, I took your xfail implementation, it works nicely 🍺

@bouweandela bouweandela merged commit bbb73b0 into master Apr 20, 2020
@bouweandela bouweandela deleted the make_ncl_or_diag-tests_not_fail branch April 20, 2020 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants