Add regression tests for client module

[JonghoKim-jj]

Closes #263

Objective

This PR adds regression tests for the client module to verify current behavior and prepare for future security updates.

What this PR introduces

1. Scenario-driven Regression Coverage
A comprehensive set of test cases mapping various TLS certificate states and edge cases.
This ensures that the module's behavior—especially regarding certificate bundle handling—is deterministic and verified.

2. Tests as "Executable Specifications"
We are introducing tests that define not just current behavior, but desired behavior.

• Regression Tests: Verify the current implementation works as expected.
• Pending Requirements: A subset of tests is written ahead of implementation to codify our alignment with Modern Security Standards (e.g., enforcing full-chain certificates and strict SAN validation).

3. Zero CI Noise (Stability by Design)
Tests for features/validations not yet implemented are marked with #[ignore] and prefixed with pending_.

• Benefit: This keeps the requirements visible in the codebase without causing CI failures.
• Roadmap: These tests serve as a clear "todo list" for future PRs, allowing us to enable them incrementally as we harden the implementation.

Context & Rationale: Why is the test suite so large?

While client.rs is currently concise (~90 lines), it manages security-sensitive invariants (TLS).
The current implementation has minimal validation, which increases the risk of silent misconfiguration.
We invested in a comprehensive test suite (1000+ lines) to:

• Pin down invariants:
  Lock in correct behavior before refactoring or expanding validation logic.
• Expose hidden complexity:
  Make subtle correctness rules (e.g., CA mismatches, chain ordering) explicit in code rather than relying on tribal knowledge.

Implementation Notes: rstest

This suite is implemented using the rstest crate as a lightweight way to express fixtures and parameterized cases in Rust. We chose it for:

• Scalability & Maintainability:
  It allows us to cover the combinatorial explosion of TLS edge cases with minimal boilerplate. Adding new scenarios becomes a data-entry task rather than writing new functions.
• Ecosystem Trust:
  rstest is widely adopted in the Rust community (55M+ downloads on Crates.io).
• Industry Compatibility:
  rstest integrates seamlessly with tokio::test, allowing us to write parameterized async tests without friction.
  Additionally, googletest-rust explicitly mentions interoperability with rstest, confirming it fits well with professional, async-heavy workflows.

Future Work

• Validation Strategy: Currently, the client module lacks specific validation functions. We need to discuss and design a proper validation layer to enforce the security rules defined in these tests.
• Integration Testing: This PR focuses on unit-level invariants. Subsequent PRs will add integration tests to validate end-to-end client connection flows.
• Enabling Pending Tests: We will progressively implement strict validation logic (Full-chain/SAN) and enable the pending tests.

[JonghoKim-jj]

@pott-cho Would you mind reviewing this PR?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 13.04%. Comparing base (9f1fb86) to head (5f63aef).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #274      +/-   ##
==========================================
+ Coverage   10.96%   13.04%   +2.07%     
==========================================
  Files           7        7              
  Lines         529      529              
==========================================
+ Hits           58       69      +11     
+ Misses        471      460      -11     

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

[pott-cho]

Since client.rs is a module responsible for generating certificates, testing handshake behavior in this location does not seem appropriate. Please remove the corresponding test.

[JonghoKim-jj]

Thanks for the feedback. I see your point regarding the module's responsibility.

However, I'd like to clarify that client.rs is not responsible solely for certificate parsing; it also exports the config() function, which constructs the final quinn::Endpoint with a specific TLS configuration.

The intention behind these tests is to serve as regression tests for the config() logic itself. We need to verify that the generated Endpoint actually works with the provided certificate chains, and some issues (like chain ordering, missing SANs, or trust validation) can only be detected during an actual TLS handshake.

Since this crate (crusher) is structured as a binary crate without a lib.rs, moving those tests to the tests/ directory would require exposing internal modules or restructuring the project. Therefore, I placed them here to ensure the client configuration is validated within the module's scope.

Do you think we should keep this validation here to ensure reliability, or would you still prefer a different approach given the structure?

[pott-cho]

I understand your reasoning. Given that the overall approach of the test code is to thoroughly cover as many cases as possible, having these tests located here also seems reasonable.

That said, there appears to be a significant amount of duplicated logic across the handshake tests. To improve readability and maintainability, it would be good to extract the shared parts into a helper function.
For example, you might consider factoring the common logic into a function like run_handshake.

[JonghoKim-jj]

Thanks for your feedback!
I will split common logic.

[JonghoKim-jj]

Done.

[sophie-cluml]

Since we should handle changes to the production code with great caution while adding tests, could you extract production code changes into a separate issue and handle it in a separate PR please?

[JonghoKim-jj]

Since we should handle changes to the production code with great caution while adding tests, could you extract production code changes into a separate issue and handle it in a separate PR please?

Done.
I forgot removing them. Thanks for the feedback!

[JonghoKim-jj]

@sophie-cluml I changed the visibility of REQUIRED_GIGANTO_VERSION to pub(crate) to use it within the handshake test logic. However, this change caused the code coverage to drop, resulting in a CI failure.

Should I add a trivial test case verifying this constant to satisfy the checker, or would you prefer adjusting the CI threshold to allow for small drops?

[sophie-cluml]

I noticed there are quite a few #[ignore] tests in this PR. Since they take up a significant number of lines, it can be distracting for reviewers to review code that isn't intended to be active yet. It also looks like some fixtures are only used by these ignored tests.

While there are several ways to handle this, I think the cleanest approach would be to remove the ignored tests and their associated fixtures from this PR entirely and track them in a separate issue. If that’s not feasible, completing the tests now or moving them into a separate commit would also work.

[sophie-cluml]

@sophie-cluml I changed the visibility of REQUIRED_GIGANTO_VERSION to pub(crate) to use it within the handshake test logic. However, this change caused the code coverage to drop, resulting in a CI failure.

Should I add a trivial test case verifying this constant to satisfy the checker, or would you prefer adjusting the CI threshold to allow for small drops?

Personally, I don't think adding a trivial test is necessary, nor do we need to adjust the CI threshold. Maintainers evaluate these drops on a case-by-case basis to determine whether meaningful tests are actually missing or if the drop is acceptable in merging process. However, if the repository maintainer has a different view, please follow his guidance. cc @kimhanbeom

[JonghoKim-jj]

Reply to #274 (review):

I removed all pending tests and fixture only used by those tests.
For later issues, I've just pushed a commit to branch leo/backup-remove-pending-tests for backup purpose.

[JonghoKim-jj]

@pott-cho @sophie-cluml

Following the review in #274 (review), I initially removed all functions and fixtures prefixed with pending_.

However, after giving it some more thought, I've changed my mind.
I believe we should restore the fixtures for the handshake test cases.

The reasons are as follows:

• crusher currently lacks validation logic, and there has been no discussion regarding which module should handle it.
• We rely entirely on crates like rustls for TLS certificate verification.
• If the handshake succeeds with an invalid certificate, it is a "BUG".
  Although the actual fix (validation logic) will be addressed in a separate Issue/PR, we must keep these tests here to ensure the bug is detected. (and does not fail silently)

Does this sound reasonable to you?

[pott-cho]

@JonghoKim-jj
Our current goal is test automation. If a pending test cannot be validated during the CI process, it would be better not to add it.
Additionally, rather than writing test code in advance for code that will be implemented later, it would be more appropriate to write the test code together when implementing that code in a separate issue.

[JonghoKim-jj]

@pott-cho That makes sense to me.
Instead of adding a pending test,
I will open an issue after this PR is merged.

[pott-cho]

@JonghoKim-jj
Yes, creating the issue would be a good idea.

[sophie-cluml]

Thanks for the comprehensive test suite. Upon reviewing, I found a few points for your consideration:

• CA loading in to_ca_certs: It currently loads only the first certificate. I think we should eventually load the entire chain to support CA bundles. Could you please track this in a separate follow-up issue?

• Safety in Certs::try_new: assert! can cause panics on invalid input, which contradicts our panic handling policy. Could you please address this hardening in a separate follow-up PR?

• (request for change) CertHintForServer comments: Some comments seem to contradict the actual test logic. For example, fullchain_root_cert acts as a Client CA in the code but is documented as a server issuer. Also, the name fullchain_root_cert feels a bit awkward to me, since a root certificate is a trust anchor, not a chain.

[JonghoKim-jj]

@sophie-cluml I've created 2 issues: #279 #280

[JonghoKim-jj]

Based on comment #274 (review),
I made few changes:

• Renamed struct: CertHintForServer -> ServerCertsSetup
• Renamed field: fullchain_root_cert -> client_trust_root
• Renamed field: first_ca_cert, first_ca_key_pair -> server_issuer_cert, server_issuer_key
• Renamed variables and parameters: hint -> cert_setup
• Updated comments to accurately describe the fields.