Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

POUNDERS/MATLAB GitHub Testing Action #146

Open
wants to merge 83 commits into
base: develop
Choose a base branch
from
Open

POUNDERS/MATLAB GitHub Testing Action #146

wants to merge 83 commits into from

Conversation

jared321
Copy link
Collaborator

@jared321 jared321 commented Apr 25, 2024

The work associated with the branch has been finished. The following will be done as part of a self-review of the PR

  • Review of all changes
  • Confirm that both layers of tox tools are still functioning correctly.
  • Confirm that all MATLAB tests are documented with correct setup information.
  • Confirm that each MATLAB test can be run independently.
  • Confirm that all MATLAB tests leave the path unaltered if successful.
  • Confirm that all MATLAB tests can be run from different locations.
  • Confirm that executing runtests in MATLAB from the clone's root will run all MATLAB tests in repo and that a single, full coverage report can be generated.
  • Break Python and MATLAB tests and confirm errors reported with tox, local MATLAB testing, and GH actions.
  • Intentionally inject formatting error to see that newly modernized flake8 and black actions fail with useful information.
  • Alter code coverage for both Python and MATLAB and confirm changes with tox, local MATLAB testing, GH actions, and CodeCov.
  • Download and sanity check coverage report artifacts.
  • Review action logs for correctness.

All manual MATLAB testing was done on GCE/compute-004 with MATLAB R2023b.

@jmlarson1
The miss_hit action presently logs the following without the action indicating failure

In pounders/m/general_h_funs/emittance_combine.m, line 1
| function [G, H] = emittance(Cres, Gres, Hres)
|                   ^^^^^^^^^ check (low): function name does not match the filename emittance_combine [filename_primary_entity_name]
MISS_HIT Lint Summary: 57 file(s) analysed, 1 check(s)

@jmlarson1 @mmenickelly Can you both please review this work.

pounders.m and its tests often add folders to the MATLAB path but do so using
relative paths, which can influence from where you can call the code.  The
changes here adopt a different path strategy using paths relative to the file
that is adding to the path.  This is related to Issue #127.  A second goal is
to make it so that users don't have to add too many paths.

Confirmed that the full test suite can be run as detailed in its inline
documentation.  This includes creating an HTML coverage report.  Confirmed that
changes to the path made by tests are not present upon termination.  Ran each
single test individually with a clean MATLAB environment to confirm that they
run through with and only with the documented path requirements.  Confirmed
that they leave the path as found upon entry.
The output of this file can vary based on the local compute environment used to
run the test.  The differences do not necessarily indicate a failure or problem
with the test.  As such there is no single baseline and we remove the file.
@jared321 jared321 self-assigned this Apr 25, 2024
@coveralls
Copy link

coveralls commented Apr 25, 2024

Pull Request Test Coverage Report for Build 8854673898

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+4.4%) to 100.0%

Totals Coverage Status
Change from base Build 8787010077: 4.4%
Covered Lines: 15
Relevant Lines: 15

💛 - Coveralls

The matrix seems to work OK, so let's just stick to Ubuntu for now.
Changes analogous to those made for POUNDERS in commit ba4fbb1.  Tested in
similar way.  See reference commit's message for more details.  Additionally
confirmed that if I run MATLAB tests with coverage enabled from the root
folder, that all MATLAB tests in IBCDFO were run and all results included in a
single coverage report.
I am getting a Commit creating failed: ["Service not found: none"] failure in
CodeCov.  Some issues online make it sound like this is due to git not having
the correct safe directory set.  The checkout action is setting safe
directories to both of the repos.  However, these are not being set for the
default folder due to my custom installation of two repos.  Setting the default
workspace as safe to see if this helps.  This is where the reports are and
where code cov is running from (AFAIK).
Copy link

codecov bot commented Apr 26, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@jmlarson1
Copy link
Collaborator

jmlarson1 commented Jun 27, 2024

@jared321 I have resolved the issue

In pounders/m/general_h_funs/emittance_combine.m, line 1
| function [G, H] = emittance(Cres, Gres, Hres)
|                   ^^^^^^^^^ check (low): function name does not match the filename emittance_combine [filename_primary_entity_name]
MISS_HIT Lint Summary: 57 file(s) analysed, 1 check(s)

I also have resolved issue #151

@jared321 jared321 changed the title DRAFT: POUNDERS/MATLAB GitHub Testing Action POUNDERS/MATLAB GitHub Testing Action Jul 1, 2024
@jmlarson1
Copy link
Collaborator

@jared321 Is it possible to see which files are covered at what levels? All I see is 93.78% here:
https://app.codecov.io/gh/POptUS/IBCDFO/pull/146?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=POptUS

Am I missing something?

@jared321
Copy link
Collaborator Author

@jmlarson1 I believe that the limited amount of information on that page is due to the fact that CodeCov is being introduced on this branch. As mentioned above and on that page, more information should be shown after this branch makes its way into main (TBC).

I am able to see the breakdown after poking around and looking at my branch:
https://app.codecov.io/gh/POptUS/IBCDFO/tree/145MatlabCI

Ran with BenDFO in default location and saw it pass.  Moved clone to different
location and saw it fail.  Set BenDFO env var to wrong path and saw it fail.
Set env var to correct location and saw it pass.
@wildsm
Copy link
Collaborator

wildsm commented Aug 5, 2024

As discussed, we should hold off on this until the cell2mat(allcomb(-)) have been eliminated.

This will mean that allcomb is not referenced in the readme and will allow the tests/actions to complete without the additional external dependency.

If all goes well, this would be addressed in Issue #157.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants