-
Notifications
You must be signed in to change notification settings - Fork 3
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
318: update tests/data for MTD 3d cluster #334
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good.
I noticed many of the GitHub Actions checks failed, so I reran them and they succeeded.
I checked the SonarQube report and it looks like it is reporting only 40.7% code coverage. The code coverage run in GHA shows 68% coverage. I thought you were closer to 80%? Could you check on this and see what is going on? Perhaps some files are being excluded/included in the report when they should not be? The list of files to ignore in SonarQube can be found in the sonar-project.properties file.
@georgemccabe I've added some coverage and pytest config in a new pyproject.toml file, this allows us to run a single Please let me know if you're happy with the exclusions, in particular |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those edits. I see that both the GHA code coverage report and SonarQube both show 81% coverage.
Regarding the exclusion of METdbLoad/sql
, I am not familiar enough with METdbLoad to know if these scripts should be ignored or not. @bikegeek or @mollybsmith-noaa, do either of you know if these scripts are used elsewhere, e.g. by METviewer or METexpress? If so, we may want to include them in the tests.
The code under the METdbLoad/sql are all sql "scripts" used to update the
db schema and for loading the schema prior to loading data into a
database. I believe we can safely exclude the METdbLoad/sql from the GHA
coverage and SonarQube scans.
…On Tue, Sep 17, 2024 at 2:32 PM George McCabe ***@***.***> wrote:
***@***.**** approved this pull request.
Thanks for making those edits. I see that both the GHA code coverage
report and SonarQube both show 81% coverage.
Regarding the exclusion of METdbLoad/sql, I am not familiar enough with
METdbLoad to know if these scripts should be ignored or not. @bikegeek
<https://github.com/bikegeek> or @mollybsmith-noaa
<https://github.com/mollybsmith-noaa>, do either of you know if these
scripts are used elsewhere, e.g. by METviewer or METexpress?
—
Reply to this email directly, view it on GitHub
<#334 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4UJHWGRV5ATDU2A7CV5ILZXCGXNAVCNFSM6AAAAABOKHPECGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMJRGAYDSMBZGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I just looked myself and fully concur with Minna!
…On Tue, Sep 17, 2024 at 3:24 PM bikegeek ***@***.***> wrote:
The code under the METdbLoad/sql are all sql "scripts" used to update the
db schema and for loading the schema prior to loading data into a
database. I believe we can safely exclude the METdbLoad/sql from the GHA
coverage and SonarQube scans.
On Tue, Sep 17, 2024 at 2:32 PM George McCabe ***@***.***>
wrote:
> ***@***.**** approved this pull request.
>
> Thanks for making those edits. I see that both the GHA code coverage
> report and SonarQube both show 81% coverage.
>
> Regarding the exclusion of METdbLoad/sql, I am not familiar enough with
> METdbLoad to know if these scripts should be ignored or not. @bikegeek
> <https://github.com/bikegeek> or @mollybsmith-noaa
> <https://github.com/mollybsmith-noaa>, do either of you know if these
> scripts are used elsewhere, e.g. by METviewer or METexpress?
>
> —
> Reply to this email directly, view it on GitHub
> <
#334 (review)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AA4UJHWGRV5ATDU2A7CV5ILZXCGXNAVCNFSM6AAAAABOKHPECGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMJRGAYDSMBZGA>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#334 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHBWGZPIWXOLMKP6XXPYTF3ZXCMYDAVCNFSM6AAAAABOKHPECGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJWHE3DCNRYGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Pull Request Testing
Describe testing already performed for these changes:
Tests run locally
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
None
[x Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes or No]
Do these changes include sufficient testing updates? [Yes or No]
yes
Will this PR result in changes to the test suite? [Yes or No]
yes, increase to 83% coverage
Do these changes introduce new SonarQube findings? [Yes or No]
If yes, please describe:
Please complete this pull request review by [Fill in date].
Pull Request Checklist
See the METplus Workflow for details.
Select: Reviewer(s) and Development issue
Select: Milestone as the version that will include these changes
Select: Coordinated METplus-X.Y Support project for bugfix releases or METdataio-X.Y.Z Development project for official releases