#1194 - fix str(err) -> str(err.value)

[AdamVoysey]

fixes #1194 - change instances of str(err) to str(err.value)

[codecov]

Codecov Report

Merging #1195 (ee052c3) into master (e96a049) will increase coverage by 0.00%.
The diff coverage is 95.95%.

@@           Coverage Diff           @@
##           master    #1195   +/-   ##
=======================================
  Coverage   98.86%   98.87%           
=======================================
  Files         209      210    +1     
  Lines       32673    32716   +43     
=======================================
+ Hits        32303    32348   +45     
+ Misses        370      368    -2     

Impacted Files	Coverage Δ
src/psyclone/tests/gocean1p0_build.py	36.00% <0.00%> (ø)
src/psyclone/tests/lfric_build.py	86.20% <ø> (ø)
src/psyclone/tests/utilities.py	63.52% <60.00%> (+0.95%)	⬆️
src/psyclone/alg_gen.py	100.00% <100.00%> (ø)
src/psyclone/configuration.py	100.00% <100.00%> (ø)
src/psyclone/errors.py	100.00% <100.00%> (ø)
src/psyclone/parse/utils.py	100.00% <100.00%> (ø)
src/psyclone/psyir/backend/visitor.py	100.00% <100.00%> (ø)
src/psyclone/psyir/symbols/symbol.py	100.00% <100.00%> (ø)
...lone/psyir/transformations/transformation_error.py	100.00% <100.00%> (ø)
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e96a049...ee052c3. Read the comment docs.

[arporter]

Thanks for fixing these Adam. I think it raises two things though:

1. The tests on GitHub Actions are not picking up the incorrect usage, even in tests that (presumably) are being run. Presumably we can change something so that they do?
2. Some of the test utility code is not covered by the tests. That's not in the scope of this PR though so I'll create a new issue.

[AdamVoysey]

I have managed to reproduce the difference in behaviour for the two systems, and it is dependant on the version of pytest used. I have further tracked it down to the particular change that cased this - pytest-dev/pytest#5934.

What appears to be happening is on the older versions of pytest, the name of the exception class is used within str(err). However in the newer versions, the __repr__ method of the underlying exception is used. As none of the Psyclone exceptions define __repr__, we instead fall back to the inherited parent Exception.__repr__ - which includes the error message within it. The assertions therefore work by accident in this case, as they are always able to match the error message for the exception.

To combat this, I have added a __repr__ to each of the Psyclone exception classes which is the same as that which pytest would previously have used. I have additionally added a new unit test which walks the Psyclone code to find all the exceptions, then ensures they define an appropriate __repr__.

With these changes, the assertions pass when str(err.value) is used, across all pytest versions. Furthermore, there are failures if only str(err) is used across all pytest versions.

Hopefully this is an acceptable solution to the problem.

[arporter]

Thanks for taking the time to dig into this and find the root cause of the problem, it's much appreciated.
Although the changes fix the problem, I think it might be better to refactor things a bit and introduce an abstract 'base' class PSycloneException which all of our other exceptions should subclass. I think we could just then implement repr in this base class as:

def __repr__(self):
    return type(self).__name__ + "()"

and therefore not need it anywhere else. Would you be happy to do this or would you prefer to create a new Issue for someone else to tackle?

I like the idea of the new test but found it hard to follow and codecov (https://app.codecov.io/gh/stfc/PSyclone/compare/1195/diff) points out that there are a few lines not covered. I had a play and came up with something a bit simpler (still using your all_sub_exceptions() routine):

def import_submodules(package, recursive=True):
    """ Import all submodules of a module, recursively, including subpackages

    :param package: package (name or actual module)
    :type package: str | module

    :rtype: dict[str, types.ModuleType]
    """
    if isinstance(package, str):
        package = importlib.import_module(package)
    results = {}
    for loader, name, is_pkg in pkgutil.walk_packages(package.__path__):
        full_name = package.__name__ + '.' + name
        if "test" not in full_name:
            results[full_name] = importlib.import_module(full_name)
            if recursive and is_pkg:
                results.update(import_submodules(full_name))
    return results

if __name__ == "__main__":

    mods = import_submodules("psyclone")
    for mod in mods:
        package = importlib.import_module(mod)
    exceptions = all_sub_exceptions(Exception)
    for exc in exceptions:
        if "psyclone." in str(exc):
            print(exc)

[AdamVoysey]

Sorry this took so long to respond to - I was called away to work on other things. I have now simplified the test and added a new abstract base class for the exceptions as suggested.

[arporter]

Thanks very much for re-factoring Adam.
Just some minor things to tweak and I think we should call it PSycloneError for consistency with other classes (apologies if I suggested PsycloneError in the first place). Once that's done this can be merged.

[arporter]

Could we now have PSycloneError instead of Exception on the line above and then remove this line entirely?

[AdamVoysey]

In principle yes - but I think I would argue this should remain as it is to make the test more robust. If a future change added an exception that (accidentally) did not inherit PSycloneError, it would be still be picked up by the current formulation; however it would be missed if suggested change was made.

[arporter]

Actually, could we therefore go one better and check that every exception we find is an instance of PSycloneError. Should just be one or two extra lines?

[arporter]

Please could you add:

# For AutoAPI documentation generation
__all__ = ["TransformationError"]

to the bottom of this file.

[arporter]

Thanks for doing this. Unfortunately I've discovered that we now get a truckload of warnings when building the reference guide I've asked a question on the sphinx user group (https://groups.google.com/g/sphinx-users/c/vuW6OOb96Yo) so we'll see what they say.

[arporter]

Please could you just comment-out this __all__ line for now with a comment to say that it causes 'more than one target for cross-reference' warnings when building the reference guide and add a TODO referring to #1280.

[AdamVoysey]

I think I've made all the requested changes except the all_sub_exceptions(Exception) -> all_sub_exceptions(PSycloneError) as I think this will make the test more fragile. Happy to change it if you disagree with this reasoning.

[arporter]

Very, very nearly there now.
Please could you extend the exception testing to check that all exception classes defined by PSyclone sub-class PSycloneError.
There's also a problem with warnings generated when building the documentation. This is not your problem though - I've created #1280 for it and just ask that you add a TODO referencing it.

[arporter]

Please could you also bring this branch up-to-date with master.

[AdamVoysey]

Changes now made

[arporter]

All requested changes have been made.
All tests and examples OK with compilation.
Will proceed to merge.