Matlab interface (new) test suite

[ssun30]

Changes proposed in this pull request

• Add unit tests for the new Matlab interface.
• Add those tests into CI workflow.

If applicable, fill in the issue number this pull request is fixing

Partially address Cantera/enhancements#177

If applicable, provide an example illustrating new features this pull request is introducing

This PR will introduce a test suite using Mathwork's testing frameworks for the new Matlab interface. It will utilize a class-based structure for running tests on each module within the interface.

It will be much more extensive than the legacy interface's test suite, which only included tests on a limited set of methods within the ThermoPhase class only. The aim is to replicate all tests that are present in the Python interface's test suite as long as the class and method exists.

Checklist

• The pull request includes a clear description of this code change
• Commit messages have short titles and reference relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• Style & formatting of contributed code follows contributing guidelines
• The pull request is ready for review

[rwest]

Some suggestions

[speth]

Thanks for sharing the work-in-progress here, @ssun30. I'd like to suggest a pause on adding new tests so we can discuss structure and functionality of this test suite and limit the scope of this PR at this time. I think the first priority should be getting the tests to actually run and indicate success/failure as the status for the corresponding GitHub Action. The previously "successful" action (here) did not actually run any of the tests that you wrote.

I also wanted to comment on a statement made in the initial post:

The aim is to replicate all tests that are present in the Python interface's test suite as long as the class and method exists.

I don't think this is quite the correct goal for the Matlab test suite. I would suggest that the aim should be to have unit tests covering everything that is unique to the Matlab toolbox, but not to duplicate tests that are mainly about testing underlying functionality of the C++ library, which is true of quite a bit of the Python test suite.

[ssun30]

Thanks for sharing the work-in-progress here, @ssun30. I'd like to suggest a pause on adding new tests so we can discuss structure and functionality of this test suite and limit the scope of this PR at this time. I think the first priority should be getting the tests to actually run and indicate success/failure as the status for the corresponding GitHub Action. The previously "successful" action (here) did not actually run any of the tests that you wrote.

Hi @speth , the Matlab Github Action was looking for the wrong folder for the Matlab tests in CI#4006. This was fixed in the subsequent commit and the test suite was running correctly (and failed as expected) in CI#4007. In a later commit, I changed the qualification methods from verification to assumption so the CI workflow would label tests that failed due to verification (no errors but returning wrong results) as filtered. This is only temporary to see whether the CI would run to completion, which it did as in CI#4011.

I also wanted to comment on a statement made in the initial post:

The aim is to replicate all tests that are present in the Python interface's test suite as long as the class and method exists.

I don't think this is quite the correct goal for the Matlab test suite. I would suggest that the aim should be to have unit tests covering everything that is unique to the Matlab toolbox, but not to duplicate tests that are mainly about testing underlying functionality of the C++ library, which is true of quite a bit of the Python test suite.

Sorry the statement I made here was not my intent. I was not trying to replicate the entire Python test suite, but rather test all the features that are present in the Matlab interface, but using the same exact parameters (for example temperature, pressure, expected values, and edge cases) for consistency.

[ssun30]

@rwest and I have discussed about how the test suite would be merged. As it stands, the complete test for each class could be over 500 lines long. So to not make the PR overly big, we are leaning towards introducing tests incrementally with several PRs. And this PR will include two test classes and the main objective is to make sure the Github Actions run them properly.

Also, as I'm making the tests, I spotted some bugs that result in verification failures (wrong values returned). Some of these are due to inconsistent basis (molar vs. mass), some due to incorrect indexing (in Matlab integer indices start from 1 instead of 0), some due to incorrect error handling on the Clib side. For now I temporarily filtered these failed tests out so they only count as incomplete so the CI could run to the end. But ideally I would like to merge those patches so all tests could pass before merging the test itself. So, as we incrementally introduce more tests, we also merge a separate PR that address the test failures. The exception would be for fatal errors that I would patch immediately upon discovery, such as #1498 .

Would this be a viable way forward?

[speth]

I think it's worth distinguishing between this PR, which establishes the structure and patterns for testing the experimental Matlab toolbox, and subsequent PRs that make the test suite more comprehensive.

For this PR, I would suggest:

• Do not add any test cases beyond what you've already implemented here.
• If there are test cases that would not pass (or crash outright), filter them and leave a short comment noting the failure. I believe the idiomatic way of doing this in Matlab's unittest package is by calling self.assumeFail() at the start of the test.

For subsequent PRs, I think each one should either:

1. Add new test cases that pass (possibly requiring some minor code changes) and/or add disabled test cases that would not currently pass (if you don't immediately have a fix or the fix itself is complex enough to warrant its own PR).
2. Fix some errors and enable previously-disabled test cases that now succeed.

I would specifically like to avoid the pattern of #1498, where we have a bug fix that is not verified by any automated testing. I think this approach is error prone and introduces needless complexity on your end managing separate branches that are logically related.

[speth]

Thanks for working on this, @ssun30, and keeping this PR to a manageable size. I think most of the comments here should be relatively easy to address.

Besides the specific comments below, I have one general recommendation for all of the test methods: I think most if not all of your test checks should be using the verifyXyz form rather than the assumeXyz form, with the exception of using assumeFail to indicate known failures.

[speth]

The documentation for the run-tests action says this should be a path "relative to the project root folder", that is just test/matlab_experimental. Did you try that and have problems? I'm wondering if this is behind the warnings in the logs related to files that are outside this path and shouldn't be examined at all as part of this test suite, for example:

[Warning: "/home/runner/work/cantera/cantera/test/matlab/TestImport.m" was
  excluded.
  Caused by:
      The specified superclass 'TestCase' contains a parse error, cannot be found
      on MATLAB's search path, or is shadowed by another file with the same name.]

and

[Warning:
  "/home/runner/work/cantera/cantera/ext/HighFive/deps/catch2/projects/XCode/OCTest/OCTest/TestObj.m"
  was excluded.
  Caused by:
      File:
      /home/runner/work/cantera/cantera/ext/HighFive/deps/catch2/projects/XCode/OCTest/OCTest/TestObj.m
      Line: 1 Column: 1
      Invalid use of operator.]� 
  [> In matlab.unittest.internal/InvalidTestFactory/createSuiteImplicitly (line 49)

[speth]

Thanks for taking care of the suggested changes, @ssun30. I think this is a solid foundation for continuing to build out the new Matlab interface and test suite.