Apply pytest unit testing

[Sichao25]

Close #278 . I put the tests in a separate folder called pytests to locate tests easier. Let me know if it is preferred to keep both python and c++ tests in one folder.

[jacobmerson]

@Fuad-HH do you know what's standard with other packages for the python tests? I.e., are they typically in /test, /test/python, etc.?

[jacobmerson]

I know we briefly discussed test coverage. Is there a convenient way for doing test coverage on python? Going through this PR, I'm seeing a bunch of stuff missing.

Note: that would be a separate PR, and unless it's trivial, not prioritized over other things.

Also, not to be fixed in this PR, but I realized I forgot to fix the python API to the "field"/"function" naming scheme we introduced in #336 , see #359

[jacobmerson]

I'm not too familiar with pytest, so pardon the naive question. Is this some sort of python way of creating the library and keeping it in scope for the whole test suite?

[Sichao25]

I think using a fixture with scope="session" is one way to share the same library instance across all tests. No sure if it is the most standard way. If I understand correctly, it doesn't matter whether we use yield or return in the current code. However, yield seems like a better practice because it leaves room for cleanup or finalization in the future. Technically, we could perform some finalization after the yield, which may or may not help with the existing library lifecycle issue, although I haven't figured out the solution yet.

[jacobmerson]

Why does this session fixture return, but the lib one yields?

[jacobmerson]

I'm not 100% sure, but should this be included in the pyproject.toml

[Sichao25]

I think that's correct.

[jacobmerson]

Possibly needed for pytest, but naively, I would think we may want longer tracebacks to help debugging?

[Sichao25]

I manually fail an assertion; without the option I get:

______________________________________ TestOmegaHField.test_field_methods[2-2-1] _______________________________________

self = <pytests.test_omega_h_field.TestOmegaHField object at 0x7f4d04490e50>
world = <PyOmega_h.Comm object at 0x7f4d745082f0>, dim = 2, order = 2, num_components = 1

    @pytest.mark.parametrize("dim, order, num_components", [
        (2, 1, 1), (2, 2, 1),
    ])
    def test_field_methods(self, world, dim, order, num_components):
        """Create an Omega_h-backed Field and exercise the public Field API."""
        mesh = self._build_mesh(world, dim)
        factory = pcms.LagrangeFunctionSpace.from_mesh(
            mesh, order, num_components, pcms.CoordinateSystem.Cartesian
        )
        field = factory.create_field()

        assert field.get_num_components() == num_components
        assert field.get_num_dof_holders() > 0

        coords = field.get_dof_holder_coordinates()
        assert coords.shape[0] == field.get_num_dof_holders()
>       assert coords.shape[1] == 5
E       assert 2 == 5

pcms/pytests/test_omega_h_field.py:38: AssertionError
=============================================== short test summary info ================================================
FAILED pcms/pytests/test_omega_h_field.py::TestOmegaHField::test_field_methods[2-2-1] - assert 2 == 5

With the shorter output I get:

pcms/pytests/test_omega_h_field.py:38: in test_field_methods
    assert coords.shape[1] == 5
E   assert 2 == 5
=============================================== short test summary info ================================================
FAILED pcms/pytests/test_omega_h_field.py::TestOmegaHField::test_field_methods[2-2-1] - assert 2 == 5

I'm not sure if this is the case for all failures, but I don't find the short traceback from pytest particularly harmful.

[Fuad-HH]

@Fuad-HH do you know what's standard with other packages for the python tests? I.e., are they typically in /test, /test/python, etc.?

For most projects I have seen where python bindings are used, the python tests are collected in a specific directory rather than being in the test directory. I think test/python is better.

[jacobmerson]

I agree with Fuad about the location. It makes more sense to me to have them in test/python

[Fuad-HH]

I have added some small suggestions.

[Fuad-HH]

Should we hardcode these options here? If -v , --tb, and -p no:cacheprovider are needed for CI/CD, they can be easily added there in the pytest run. --strict-markers looks good to me.

In CI/CD, the following can be used:

pytest -v --tb=short -p no:cacheprovider

[Sichao25]

I think it depends on what default options we want for the pytest command. Not all of them are specifically meant for CI. I feel --strict-markers looks good in any case. -p no:cacheprovider is specific to CI, so we could remove it here. I don't have a preference on the other options. Do you find it easier to test without verbose and shorter traceback?

[Fuad-HH]

I mean we can leave it upto the user and they are free to use whichever option they want when running pytest.

[Sichao25]

Sounds good. Just removed that.

[jacobmerson]

@Sichao25 can you also add this to the self-hosted runner? I think it would be good if the python tests run in GPU configuration since Fuad already found some bugs in the python API related to GPUs (and we will need to add support for PyTorch device tensors for our LEADS collaboration).

[Sichao25]

@jacobmerson Sure. That may also require the update of omega_h dependency with -DOmega_h_USE_pybind11=on.

[jacobmerson]

Rebuilding all the AMPERE80 deps.