Skip to content

Mock the actual behavior of fail_json #173

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

Merged
merged 1 commit into from
Apr 14, 2025
Merged

Mock the actual behavior of fail_json #173

merged 1 commit into from
Apr 14, 2025

Conversation

baoy1
Copy link
Contributor

@baoy1 baoy1 commented Apr 3, 2025

Ansible faile_json will call sys.exit, then abort the execution, but currently we use FailJsonException to mock fail_json, that it cannot abort the execution.

Now mock fail_json to throw SystemExit to align with Ansible behavior.

All UTs are updated to extend PowerScaleUnitBase.

Description

A few sentences describing the overall goals of the pull request's commits.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, pep8, linting, or security issues
  • I have performed Ansible Sanity test using --docker default
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • Backward compatibility is not broken

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

  • UT

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (b7de8d3) to head (bfe8972).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
- Coverage   90.54%   90.24%   -0.30%     
==========================================
  Files         145      143       -2     
  Lines       16695    16541     -154     
  Branches     2367     2365       -2     
==========================================
- Hits        15116    14928     -188     
- Misses        920      942      +22     
- Partials      659      671      +12     
Flag Coverage Δ
units 90.24% <100.00%> (-0.30%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@baoy1 baoy1 force-pushed the usr/baoy1/fail_json branch 6 times, most recently from dcac950 to 344be16 Compare April 4, 2025 12:58
@baoy1 baoy1 marked this pull request as ready for review April 7, 2025 02:16
taohe1012
taohe1012 previously approved these changes Apr 7, 2025
@vangork
Copy link
Contributor

vangork commented Apr 9, 2025

As per the Codecov report, it brings missing and partial..

@baoy1
Copy link
Contributor Author

baoy1 commented Apr 9, 2025

As per the Codecov report, it brings missing and partial..

After adopting the same behavior as fail_json, some UTs coverage indeed went down, because the previous FailJsonException is caught and thrown to upper method caller.
Before issuing the PR, I've struggled for some days to fix UT errors (logic and design, etc), add test cases and improve the coverage. For now we have 90+% coverage, that meets the criteria already, so I am not planning to make the code coverage look appealing.

RayLiu7
RayLiu7 previously approved these changes Apr 10, 2025
@vangork
Copy link
Contributor

vangork commented Apr 11, 2025

It is not the module code coverage, but the test case code coverage which is weired.

@P-Cao
Copy link
Contributor

P-Cao commented Apr 11, 2025

For codecov app, the stats are weird indeed. Maybe we can adopt a more reasonable way to calculate the coverage in future.

@baoy1 baoy1 dismissed stale reviews from RayLiu7 and taohe1012 via d7c5580 April 11, 2025 03:57
@baoy1 baoy1 force-pushed the usr/baoy1/fail_json branch 3 times, most recently from 5b4c995 to 02f4791 Compare April 11, 2025 05:49
Ansible faile_json will call sys.exit, then abort the execution, but
currently we use FailJsonException to mock fail_json, that it cannot
abort the execution.

Now mock fail_json to throw SystemExit to align with Ansible behavior.

All UTs are updated to extend PowerScaleUnitBase.
@baoy1 baoy1 force-pushed the usr/baoy1/fail_json branch from 02f4791 to bfe8972 Compare April 11, 2025 05:51
@baoy1
Copy link
Contributor Author

baoy1 commented Apr 11, 2025

Good catch. It is my fault. Used wrong indentation. Please recheck the result.

@baoy1 baoy1 requested a review from RayLiu7 April 11, 2025 07:20
@baoy1 baoy1 merged commit f402b1b into main Apr 14, 2025
19 checks passed
@baoy1 baoy1 deleted the usr/baoy1/fail_json branch April 14, 2025 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants