xenserver / status-report

Program that gathers data for xenserver host diagnostics
GNU Lesser General Public License v2.1
1 stars 9 forks source link

Initial unit tests for the new XS SNMP config filters #53

Closed BengangY closed 9 months ago

BengangY commented 9 months ago

Update by @bernhardkaindl:

Status: This adds a unit test case and extends the container-based test to test SNMP code (not > completely.

There would be more to do, e.g. to check what happens then the format of the SNMP files do not match > the filter expression. But this can be deferred to a later PR.

For this PR, I request from @BengangY that he just merges my commit into this PR branch using my sub-PR below or cherry-picks (git pull; git cherry-pick e2171045cd36b8536f8712655dcfb8affa34f650 may be enough).

Then, with no further changes, it should look good to approve it, merge it into the target feature branch, and merge the SNMP feature of collecting the SNMP config files: https://github.com/xenserver/status-report/tree/feature/snmp-status-report

My fixup commit is here:

https://github.com/BengangY/status-report/pull/1

This is the commit for you to cherry-pick:

https://github.com/BengangY/status-report/commit/e2171045cd36b8536f8712655dcfb8affa34f650

Here is the complete branch with the 4 commits for the SNMP feature (will also need squashing at the end): https://github.com/xenserver-next/status-report/tree/private/bengangy/CP-46759-fixup

It changes a function that was very easy to mis-use to check nearly nothing into a function that can only be used to assert the presence of expected oupout.

With that, it can no longer be used to make a test look like (as a quick hack) as if checking something when it does not look at the content of an output file at all (in this case, it contains an error, which is not what we like to achive in the positive case. negative testing is needed to, but we also need the positive case.)

And here is the diff between the two branches, so everyone can see it:

https://github.com/BengangY/status-report/compare/private/bengangy/CP-46759...xenserver-next:status-report:private/bengangy/CP-46759-fixup?expand=1#diff-73726d8e938c085ad9ff2315b623baf310c4e583b85eca33e33bf13124cae1f6R16


BengangY commented 9 months ago

Failed in pre-commit check for no permission to create /etc/snmp/* file. Need to investigate it.

codecov-commenter commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (feature/snmp-status-report@e8379fa). Click here to learn what that means.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## feature/snmp-status-report #53 +/- ## ============================================================= Coverage ? 85.05% ============================================================= Files ? 16 Lines ? 1981 Branches ? 0 ============================================================= Hits ? 1685 Misses ? 296 Partials ? 0 ``` | [Flag](https://app.codecov.io/gh/xenserver/status-report/pull/53/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xenserver) | Coverage Δ | | |---|---|---| | [python2.7](https://app.codecov.io/gh/xenserver/status-report/pull/53/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xenserver) | `83.53% <0.00%> (?)` | | | [python3.10.13](https://app.codecov.io/gh/xenserver/status-report/pull/53/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xenserver) | `84.12% <0.00%> (?)` | | | [unittest](https://app.codecov.io/gh/xenserver/status-report/pull/53/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xenserver) | `84.12% <0.00%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xenserver#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

BengangY commented 9 months ago

1 /etc/snmp/snmp.xs.conf format changed, so we need to replace sensitive string with the new format. 2 Add UT case for SNMP and the environment configure updates. 3 Update .coveragerc for the coverage should not cover the test files.

BengangY commented 9 months ago

Hi @bernhardkaindl I have cherry-picked your commit https://github.com/BengangY/status-report/pull/1 into this PR, please merge this PR into the master. I will refine the test cases in a later PR. Thanks!

bernhardkaindl commented 9 months ago

Squashed and merged this PR on top of the SNMP feature branch.

If you want to open a PR for merge to master:

There were significant updates to the master branch to add recoding the code coverage from the tests in tests/unit (other tests would need more updates to merge coverage files as they run bugtool in a new process).

Then, the tests in tests/unit were further improved to raise the code coverage from below 50% code coverage to above about 85%.

This was then the basis to update these 85% of the code to support Python 3.10, which the improved unit tests confirm by always still passing in GitHub CI workflows of each merged PR.

Because these were significant changes, the SNMP feature branch now have some changes that may now be conflicting with the master.

This workflow can be improved if such feature branches are rebased to the master branch when concurrent work on a feature is being re-started after updates to master were done.

Now, before further changes, it would be recommended to rebase the SNMP branch to the master branch.

Even if it could be that it is not conflicting with master yet, the more time and commits accumulate the higher that probability may be.