TraceMachina / nativelink

NativeLink is an open source high-performance build cache and remote execution server, compatible with Bazel, Buck2, Reclient, and other RBE-compatible build systems. It offers drastically faster builds, reduced test flakiness, and specialized hardware.
https://nativelink.com
Apache License 2.0
1.19k stars 117 forks source link

Update coverage for config and error #1475

Open leodziki opened 1 week ago

leodziki commented 1 week ago

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

How Has This Been Tested?

Bazel test, cargo test and pre-commit

Checklist


This change is Reviewable

leodziki commented 1 week ago

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

leodziki commented 1 week ago

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.
leodziki commented 1 week ago

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.