Update coverage for config and error

[leodziki]

Update coverage rate of nativelink-config and nativelink-error to 80~100% Passed cargo test and bazel test

Description

Currently, coverage rate has been upgraded to 80-100% in nativelink-error and nativelink-config packages.
Bazel test and cargo test both passed. And Pre-commit passed in direnv and everything is perfect.
Fixed everything about the review

Fixes #1401

Type of change

• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
• [x ] This change requires a documentation update

How Has This Been Tested?

Bazel test, cargo test and pre-commit

Checklist

• Tests added/amended
• bazel test //... passes locally

---

This change is 

[leodziki]

I would appreciate it if you could take a moment to review my pull request. Your feedback would be invaluable, and I’m particularly interested in any thoughts you have on the implementation and overall impact of the changes made.
@blakehatch, @adam-singer

[adam-singer]

Reviewed 3 of 5 files at r1, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and 3 of 5 files reviewed, and 2 discussions need to be resolved

---

nativelink-config/tests/deserialization_test.rs line 28 at r1 (raw file):

const MAP_TYPE_ERROR: &str = "invalid type: map, expected a string containing json data";

const DURATION_HUMAN_READABLE: &str = r#"{"duration": "1m 10s"}"#;

Using these consts makes looking up what the example content is a double navigation. I think they should continue to be inlined until duplication is unmanageable

---

nativelink-metric-collector/src/lib.rs line 19 at r1 (raw file):

mod metrics_collection;
pub mod metrics_visitors;

why is this public?

[leodziki]

Thanks for your review

1. The double iteration issue you mentioned is generally minor unless it significantly affects readability or productivity. Inlining test values improves readability by keeping the context readily accessible within the test code. If constants are declared separately, it requires extra scrolling or navigation, disrupting the flow. Duplication is only a concern when the same data is reused across multiple tests; for unique values, inlining is simpler and cleaner. Constant declarations become necessary when duplication complicates maintenance. Therefore, inlining is a practical approach for unique or infrequently reused values, while centralizing constants should only be considered when duplication becomes problematic.
2. This is not directly related to nativelink-config and error.
   I changed it for other test code. I made private public to test nativelink-metric-collector.
   (e.g. #[cfg(test)] mod tests in src/metrics_visitors.rs). This allows you to test private functionality directly without making it public, but it requires that your tests be in the src/ directory, not tests/.
   So I'm going to do this for code cleanliness.
   I think this way I can keep the scope of the module strictly in production, while still allowing access to the tests. If it's better to keep it private, you could consider making #[cfg(test)] and pub(crate) or #[cfg_attr(test, pub)] public only in the test configuration, but I don't think there's any other problem with making it public.
   This doesn't directly affect nativelink-error and config.

[aaronmondal]

@leodziki Thanks for finding some test cases!

b6cf659 changed the serde_utils and tests fairly significantly. Could you split off your changes to serde_utils.rs into a separate PR and integrate your test cases into the new structure?

Reviewable status: 0 of 2 LGTMs obtained, and 3 of 5 files reviewed, and pending CI: Bazel Dev / macos-13, and 2 discussions need to be resolved

[leodziki]

Hello, @aaronmondal
Thank you for your review
I will address this by splitting off my changes to serde_utils.rs into a separate PR and incorporating my test cases into the updated structure. Thank you for the feedback.