arch: Ensure compiler check catches permission errors

[gbruer15]

I added a test for #2336, but I'm not sure if it will actually error on the bug I was encountering, so I'd like to run the standard test suite to make sure.

Closes #2336

[mloubout]

Thanks!.

You can the test locally to make sure it does what you want with

py.test -vsx tests/test_error_checking.py -k test_sniff_compiler_version

(-vsx is just for verbosity)

[mloubout]

I think it needs to be

with pytest.raises(SystemExit) as e:
        sniff_compiler_version(cc)
 assert e == SystemExit

so there is a test.

[gbruer15]

I could add assert e.type == SystemExit, but pytest.raises(SystemExit) catches only a SystemExit exception, there's no point. If it's not a SystemExit exception, it won't get caught.

For an example, consider the test.py below, where non-SystemExit exceptions simply cause the test to fail.

import pytest
import sys

def test_succeeds():
    with pytest.raises(SystemExit) as e:
        sys.exit(1)
    assert e.type == SystemExit

def test_fails():
    with pytest.raises(SystemExit) as e:
        raise NotImplementedError

def test_succeeds_again():
    with pytest.raises(NotImplementedError) as e:
        raise NotImplementedError

$ python -m pytest test.py
================================== test session starts ===================================
platform linux -- Python 3.9.15, pytest-8.1.1, pluggy-1.4.0
rootdir: /home/gbruer/a
plugins: anyio-3.5.0
collected 3 items                                                                        

test.py .F.                                                                        [100%]

======================================== FAILURES ========================================
_______________________________________ test_fails _______________________________________

    def test_fails():
        with pytest.raises(SystemExit) as e:
>           raise NotImplementedError
E           NotImplementedError

test.py:12: NotImplementedError
================================ short test summary info =================================
FAILED test.py::test_fails - NotImplementedError
============================== 1 failed, 2 passed in 0.08s ===============================

[FabioLuporini]

Also: this test should be moved to some other files as this one, test_error_checking, is supposed to be for runtime (C-level) numerical and computational errors.

Perhaps test_environment?

[FabioLuporini]

sys.exit(...) is also quite brutal, perhaps we turn it into a RuntimeError?

[gbruer15]

test_environment.py seems to be making sure the test environment is usable, so this test doesn't fit there in my opinion. This test is independent of the test environment. Should there be a second test file for non-numerical runtime errors?

I agree that RuntimeError is better. I'll make that change.

[FabioLuporini]

Should there be a second test file for non-numerical runtime errors?

perhaps test_arch.py where in the future we can also add all other /devito/arch related tests?

[gbruer15]

I moved the test to test_arch.py and fixed the pep8 errors, so as far as I know, this is ready to be merged.

[mloubout]

There is benchmark setup that needs fixing and rebase, I'll do it and get it merged

[gbruer15]

I tested locally and it's fine. The issue is I'm not sure how I should consistently trigger the PermissionDenied error to ensure it's being caught.

I could make a file without execute permissions and delete it afterwards. I'm not sure if that will be portable for the OS's that Devito supports, though.

[mloubout]

Looks good to me. Not sure where else to put that test though but not critical IMO

[codecov]

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 66.18%. Comparing base (e2743e0) to head (a1dabe9).

Files	Patch %	Lines
devito/arch/compiler.py	50.00%	1 Missing ⚠️
devito/mpi/distributed.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2340       +/-   ##
===========================================
- Coverage   86.72%   66.18%   -20.54%     
===========================================
  Files         232      184       -48     
  Lines       43641    28818    -14823     
  Branches     8075     6381     -1694     
===========================================
- Hits        37848    19074    -18774     
- Misses       5084     8891     +3807     
- Partials      709      853      +144     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[FabioLuporini]

We just ened to find a better place for the test, test_error_checking is meant for something else, and perhaps we should rename it

[FabioLuporini]

Should there be a second test file for non-numerical runtime errors?

perhaps test_arch.py where in the future we can also add all other /devito/arch related tests?

[FabioLuporini]

there are a few pep8 issues to be resolved: https://github.com/devitocodes/devito/actions/runs/8525302940/job/23365391849?pr=2340