Skip to content

feat: Add facility function tests and fix missing test targets#505

Merged
goatshriek merged 5 commits intogoatshriek:latestfrom
rs0125:latest
Jun 10, 2025
Merged

feat: Add facility function tests and fix missing test targets#505
goatshriek merged 5 commits intogoatshriek:latestfrom
rs0125:latest

Conversation

@rs0125
Copy link
Contributor

@rs0125 rs0125 commented Jun 9, 2025

Improve Facility Error Handling (Fixes #456)

This PR improves error handling and test coverage for the facility-related functions in the library. It also introduces a new function test target to aid future debugging.


Code Changes

src/facility.c

  • stumpless_get_facility_string:

    • Now calls clear_error() on success.
    • Calls raise_error(STUMPLESS_INVALID_FACILITY) for invalid enums.
  • stumpless_get_facility_enum_from_buffer:

    • Calls clear_error() on success.
    • Calls raise_error(STUMPLESS_INVALID_FACILITY) for unmatched strings.
  • stumpless_get_facility_enum:

    • Now clears error if the call to *_from_buffer was successful.

Test Additions (test/function/facility.cpp)

Added tests in a new FacilityErrorHandling suite:

  • ClearsPreviousErrorOnSuccess: ensures previous errors are cleared.
  • InvalidFacilityEnumRaisesError: checks invalid enum detection.
  • InvalidFacilityStringRaisesError: checks invalid string input.

Function Target

  • Added a function-test-facility CMake target to run facility tests independently (mirroring function-test-version pattern).
  • All 8 tests pass successfully.

image


Doxygen Header Updates (include/stumpless/facility.h)

Each relevant function comment now includes:

  • @error This function clears any previous error on success.
  • @error This function sets STUMPLESS_INVALID_FACILITY on invalid input.

@codecov
Copy link

codecov bot commented Jun 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.59%. Comparing base (d2f2193) to head (f9b5fcf).
Report is 1 commits behind head on latest.

Additional details and impacted files
@@            Coverage Diff             @@
##           latest     #505      +/-   ##
==========================================
+ Coverage   90.57%   90.59%   +0.02%     
==========================================
  Files          47       47              
  Lines        4561     4571      +10     
  Branches      607      608       +1     
==========================================
+ Hits         4131     4141      +10     
  Misses        284      284              
  Partials      146      146              

☔ 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.

@goatshriek
Copy link
Owner

Thanks very much for putting this together, particularly for describing your updates so thoroughly in the PR!

As you work through these errors (the header check static analysis should help), please also remove the formatting changes from this PR. The changes should be limited to just the changes you are adding, and not reformatting if other parts of updated files.

@rs0125
Copy link
Contributor Author

rs0125 commented Jun 9, 2025

@goatshriek would it be possible to run a check now?

There was a missing header somewhere which was causing the issues (private/error.h)
Added that and that should have fixed it.
Also cleaned up all the reformatting changes.

Also is there any way I can run the CI tests on my own (like during some other contribution) before sending the commit?

@rs0125 rs0125 force-pushed the latest branch 2 times, most recently from 31fc732 to bcf0f28 Compare June 9, 2025 15:10
@goatshriek
Copy link
Owner

goatshriek commented Jun 9, 2025

Also is there any way I can run the CI tests on my own (like during some other contribution) before sending the commit?

There are two straightforward ways to do this, the first and probably easiest being to enable GitHub Actions on your fork. That should allow you to recreate the exact checks being run for the PR without waiting for a maintainer to approve the runs. You can also go through the workflow files and run the tests manually yourself in your dev environment, but this requires you to install dependencies and tooling, and is a bit more work and more error prone.

@rs0125
Copy link
Contributor Author

rs0125 commented Jun 9, 2025

Had another header file issue

This should fix it

Copy link
Owner

@goatshriek goatshriek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good! Just a few formatting changes for you to make, and this should be good to go once the headers are also worked out.

@rs0125
Copy link
Contributor Author

rs0125 commented Jun 9, 2025

That should be all. Please let me know if there's any other changes
Thanks a lot for your patience

@rs0125
Copy link
Contributor Author

rs0125 commented Jun 10, 2025

I wonder why the test scripts need reimports of headers as different instances when they're already calling scripts from the base facility.c file. The CI workflow keeps flagging it even though it runs on compilation. Is there any particular reason for having that check?

Anyway I've ammended my last commit. Pardon me for making this go on for longer than it should have.

@goatshriek goatshriek self-assigned this Jun 10, 2025
@goatshriek goatshriek added the enhancement new features or improvements label Jun 10, 2025
@goatshriek goatshriek merged commit f3666a5 into goatshriek:latest Jun 10, 2025
56 checks passed
@goatshriek
Copy link
Owner

Thanks again for sticking with this through the change requests!

The tests are checked separately for includes since they are different compilation units, and likely even use a different compiler than the .c files in the library itself (gcc vs. g++, for example). It's important to make sure that everything used in a single *.cpp or *.c file is included in it, so that there is not an implicit dependency on one header including another, which could lead to unexpected breakage. IWYU is a standard tool for enforcing this, and has more information on the philosophy, if you'd like to learn more about it. We use a custom script here for the checks, to reduce the number of dependencies used for dev, since Ruby is already used for C++ binding generation.

@rs0125
Copy link
Contributor Author

rs0125 commented Jun 10, 2025

Thanks a lot for the thorough reviews and guidance throughout! Looking forward to contributing more to this project in the future.

Really appreciate the time and feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement new features or improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add errors to facility functions

2 participants