Skip to content

test: add unit tests for environment variable bindings#747

Open
Molter73 wants to merge 1 commit into
mainfrom
mauro/chore/add-env-var-unit-tests
Open

test: add unit tests for environment variable bindings#747
Molter73 wants to merge 1 commit into
mainfrom
mauro/chore/add-env-var-unit-tests

Conversation

@Molter73
Copy link
Copy Markdown
Collaborator

@Molter73 Molter73 commented Jun 1, 2026

Description

Adds coverage for all FACT_* env-var bindings in FactCli:

  • env_vars: each env var populates the correct FactConfig field
  • env_vars_override_yaml: env vars take precedence over YAML config
  • env_vars_invalid_values: invalid values produce ValueValidation errors

Introduces EnvVar and EnvVarGuard helper types that together acquire ENV_MUTEX, set the variable, and guarantee removal on drop — even if the test panics. std::env::set_var is not safe in multi-threaded programs; the mutex serialises all env-var mutations within the suite as the least invasive available mitigation.

Closes #730

Assisted-by: Claude Sonnet 4.6

Checklist

  • Patch has a change log entry OR does not need one.
  • Investigated and inspected CI test results
  • Updated documentation accordingly

Automated testing

  • Added unit tests
  • Added integration tests
  • Added regression tests

If any of these don't apply, please comment below.

Testing Performed

Run the tests manually.

Summary by CodeRabbit

  • Tests
    • Added comprehensive test coverage for environment variable-based configuration, including validation of individual variables, override behavior with YAML configuration, and error handling for invalid values.

Adds coverage for all FACT_* env-var bindings in FactCli:
- env_vars: each env var populates the correct FactConfig field
- env_vars_override_yaml: env vars take precedence over YAML config
- env_vars_invalid_values: invalid values produce ValueValidation errors

Introduces EnvVar and EnvVarGuard helper types that together acquire
ENV_MUTEX, set the variable, and guarantee removal on drop — even if
the test panics. std::env::set_var is not safe in multi-threaded
programs; the mutex serialises all env-var mutations within the suite
as the least invasive available mitigation.

Closes #730

Assisted-by: Claude Sonnet 4.6
@Molter73 Molter73 requested a review from a team as a code owner June 1, 2026 14:09
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Enterprise

Run ID: 29007d54-6c49-41f9-beb9-ed377cb98a62

📥 Commits

Reviewing files that changed from the base of the PR and between c9e27b6 and b9ed127.

📒 Files selected for processing (1)
  • fact/src/config/tests.rs

📝 Walkthrough

Walkthrough

This PR adds concurrency-safe test infrastructure and three comprehensive test cases validating that environment variable bindings (FACT_INODES_MAX, etc.) correctly populate FactConfig fields, override YAML configuration, and properly error on invalid inputs.

Changes

Environment Variable Test Coverage

Layer / File(s) Summary
Thread-safe test harness and helpers
fact/src/config/tests.rs
Imports Mutex, MutexGuard, and error/display traits; defines global ENV_MUTEX and EnvVarGuard RAII type that removes environment variables on drop while holding the lock; provides EnvVar wrapper with set() method and Display implementation; implements with_env_var() helper that sets an env var and parses CLI into FactConfig under mutex protection to prevent test interference.
Environment variable binding tests
fact/src/config/tests.rs
env_vars test iterates over FACT_* environment variables and asserts the parsed config matches expected field values. env_vars_override_yaml test verifies that env vars override corresponding YAML-sourced config values. env_vars_invalid_values test validates that invalid env var inputs produce parse/validation errors with expected source strings.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A mutex guard hops through the test fair,
where env vars dance without a care—
locked, set, parsed, then cleaned with grace,
no race conditions in this space! 🔒✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title accurately and concisely summarizes the main change: adding unit tests for environment variable bindings.
Description check ✅ Passed The description covers the key changes, testing approach, and checklist items; however, Testing Performed section is incomplete with only 'Run the tests manually.'
Linked Issues check ✅ Passed The PR successfully implements all coding requirements from issue #730: comprehensive env-var tests (individual vars, precedence over YAML, invalid values), mutex-based serialization for race condition mitigation, and proper cleanup.
Out of Scope Changes check ✅ Passed All changes in the PR are focused on adding unit tests for environment variable bindings as specified in issue #730; no out-of-scope modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mauro/chore/add-env-var-unit-tests

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

test: add unit tests for environment variable bindings (e.g. FACT_INODES_MAX)

1 participant