(closes #2936, #1220, #2088) Fix doctests and add CI tests

[sergisiso]

No description provided.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (a7ea7cc) to head (ce724c4).

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3441   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          393       393           
  Lines        55067     55046   -21     
=========================================
- Hits         55067     55046   -21     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sergisiso]

@arporter @LonelyCat124 This PR fixes the doctests using both:
cd docs; make doctest
and
pytest -n auto --doctest-modules src/psyclone/

And enables both as checks in CI (We need both because there isn't a complete overlap between both sets and their working dir is different, so we need to be sure that paths have been normalised).

There are some doctests that didn't work, for example because they used non-existing example files. In this cases I converted the >>> to ... code-block :: which is not tested, and we can bringing them back to doctests over time.

It is ready for review.

[arporter]

Good job Sergi, it's great to have this working and the PR wasn't quite as big as I feared :-)
Only significant quibble is with the new utility method - I'm not sure it buys us much over get_invoke and wonder whether the two can be merged?

[arporter]

This could be a bit fragile. Could we make it a little better by walking over Loops?

[sergisiso]

Done.

Note that a lot of doctests like this one are currently not very interesting, they only show how to apply a transformation (which a python user should already know). What is more interesting is to show the before and after of the associated code. I suppose this was the idea behing the print(schedule.view()) but these are too lang and hard to read and they where commented in the documentation anyway (so not checked nor shown in docs).

My idea is to make a second chosing snippets of fortran that demonstrate each transformation, but it became too much for this PR. So for now I just wanted to enable the automatic testing and then we should make them apply and showcase more relevant inout/output nodes/strings.

[arporter]

That sounds good. For this PR though this file isn't showing as modified since my last review (but the OMP one is)?

[arporter]

Does that mean we should be monkeypatching or something or are those changes local to this test?

[arporter]

(Well done on fixing this BTW - presumably this was the test error we see occasionally?)

[sergisiso]

Yes, this was not isolated. But under further inspection I think we were overcomplicating a lot this test. I updated to something much simpler. Let me know what you think.

[arporter]

I asked Claude "I want to write a test in Python that checks that all classes within my package (PSyclone on github) that inherit from Exception do so via a package-specific sub-class of Exception (namely PSycloneError). What's the best way to do this?" and it came up with:

import ast
import pathlib
import pytest

# Adjust this to point at the root of the PSyclone package source
PACKAGE_ROOT = pathlib.Path(__file__).parent.parent / "src" / "psyclone"

ALLOWED_BASE = "PSycloneError"

# Names that are acceptable bases — add aliases here if PSycloneError is
# imported under a different name anywhere
EXCEPTION_BASES = {"Exception", "BaseException"}


def is_exception_base(name: str) -> bool:
    """Return True if `name` looks like a raw exception base class."""
    return name in EXCEPTION_BASES or (
        name.endswith(("Error", "Exception")) and name != ALLOWED_BASE
    )


def get_base_name(node: ast.expr) -> str | None:
    """Extract a simple name or attribute name from a base class node."""
    if isinstance(node, ast.Name):
        return node.id
    if isinstance(node, ast.Attribute):
        return node.attr  # e.g. `builtins.Exception` → `Exception`
    return None


def find_bad_exception_classes(path: pathlib.Path) -> list[tuple[str, str, int]]:
    """
    Return a list of (file, class_name, line_no) for every class that
    inherits directly from a raw exception base instead of PSycloneError.
    """
    violations = []
    source = path.read_text(encoding="utf-8")
    try:
        tree = ast.parse(source, filename=str(path))
    except SyntaxError:
        return []  # skip unparseable files

    for node in ast.walk(tree):
        if not isinstance(node, ast.ClassDef):
            continue
        # Skip PSycloneError itself
        if node.name == ALLOWED_BASE:
            continue
        for base in node.bases:
            base_name = get_base_name(base)
            if base_name and is_exception_base(base_name):
                violations.append((str(path), node.name, node.lineno))

    return violations


def all_python_files():
    return list(PACKAGE_ROOT.rglob("*.py"))


@pytest.mark.parametrize("py_file", all_python_files())
def test_exceptions_use_psyclone_error(py_file):
    violations = find_bad_exception_classes(py_file)
    assert violations == [], (
        f"Classes inheriting directly from Exception/BaseException "
        f"instead of {ALLOWED_BASE}:\n"
        + "\n".join(f"  {f}:{line} — {cls}" for f, cls, line in violations)
    )```

[sergisiso]

@arporter This is ready for another look, I provided in the comments some more explanation about why I did certain additions/deletetions in the PR that were missing for context.

[arporter]

Thanks @sergisiso, almost there now. There's just one comment that needs some more attention and we need to think a bit more about the troublesome test for PSycloneError.

[arporter]

The idea of the old test was to make sure no developer added their own Exception class that failed to extend PSycloneError. However, it wasn't nice and it did fail occasionally. I'm wondering whether we could use the inspect module instead? EDIT - I asked Claude and it recommended parsing the AST - see other comment.