Skip to content

Add prival validation to stumpless_set_entry_prival() (modification for #478 issue)#526

Merged
goatshriek merged 35 commits intogoatshriek:latestfrom
FujiwaraNaoto:add-prival-validation-to-stumpless_set_entry_prival
Sep 13, 2025
Merged

Add prival validation to stumpless_set_entry_prival() (modification for #478 issue)#526
goatshriek merged 35 commits intogoatshriek:latestfrom
FujiwaraNaoto:add-prival-validation-to-stumpless_set_entry_prival

Conversation

@FujiwaraNaoto
Copy link
Contributor

I added validation for prival in the stumpless_set_entry_prival() function. which is mentioned here

This is my very first OSS commit merge request. I would appreciate your review.

@FujiwaraNaoto FujiwaraNaoto force-pushed the add-prival-validation-to-stumpless_set_entry_prival branch from b6ecda7 to 4f2b77f Compare September 11, 2025 09:44
@goatshriek goatshriek added the bug something is broken or missing label Sep 12, 2025
@goatshriek goatshriek self-assigned this Sep 12, 2025
@codecov
Copy link

codecov bot commented Sep 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.64%. Comparing base (40b238e) to head (acb2061).
⚠️ Report is 1 commits behind head on latest.

Additional details and impacted files
@@            Coverage Diff             @@
##           latest     #526      +/-   ##
==========================================
- Coverage   90.80%   90.64%   -0.17%     
==========================================
  Files          47       47              
  Lines        4570     4576       +6     
  Branches      608      609       +1     
==========================================
- Hits         4150     4148       -2     
- Misses        276      283       +7     
- Partials      144      145       +1     

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

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 great, thank you for putting this together!

It looks like there is a missing locale header update, which caused the locale tests to fail.

The memory leak failures can likely be resolved by calling stumpless_destroy_entry_only on the entries created in the tests before stumpless_free_all.

@FujiwaraNaoto
Copy link
Contributor Author

Thanks for your reply and concrete advice.
I have modified codes.
Could you review it again?

@goatshriek
Copy link
Owner

All that remains is one more line that is too long, and some re-ordering for alphabetic ordering within the locale headers.

@FujiwaraNaoto
Copy link
Contributor Author

FujiwaraNaoto commented Sep 12, 2025

Thanks for your reply.
I have modified the codes , run ruby scripts/check_l10n.rb "include/private/config/locale/*-??.h".
and nothing is displayed, so tests will be passed.

I have to make some changes

@FujiwaraNaoto
Copy link
Contributor Author

FujiwaraNaoto commented Sep 13, 2025

I have to make some changes

modified.

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.

Excellent work, thank you for sticking with this!

@goatshriek goatshriek merged commit 844641e into goatshriek:latest Sep 13, 2025
55 of 56 checks passed
@FujiwaraNaoto
Copy link
Contributor Author

Thanks for merging my merge request.

By the way I don't understand why the results of codecov/project change before and after the commit acb2061

@goatshriek
Copy link
Owner

The codecov testing is based on the CI runs generating coverage reports, which include several different configurations. These all generate different coverage, which is good! That's why we have them.

Unfortunately, it can also mean that a coverage report that was generated halfway through the CI runs makes it look like coverage dropped when it didn't. If you're watching coverage before all of the runs are completed, you may see a report that makes it looked like it is lower, when in reality it just isn't done yet.

Some of the thread-safety tests are also non-deteriministic because of the nature of concurrency, which generates some false changes in coverage that aren't your fault. In this case that may be what's happening, since I see some journald test coverage changes, which is one of the test cases that is variable right now. That's obviously not the desired end state, I just haven't resolved it yet since it isn't critical.

All that to say, don't worry about any minor coverage changes for contributions! What's important is the coverage of your patch, which is covered 100%, and so you are good to go.

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

Labels

bug something is broken or missing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants