Skip to content

Conversation

@ClSlaid
Copy link
Contributor

@ClSlaid ClSlaid commented Nov 4, 2025

This PR adds test for config loading.

Part of #224

Summary by CodeRabbit

  • Tests
    • Added unit tests that validate parsing of a new naive TOML configuration: workspace name, cache/index directories, and three rule entries (two populated rules with patterns/append/remove lists and one empty/default rule).
  • Chores
    • Added the corresponding naive TOML configuration used by the tests as a sample config.

Signed-off-by: 蔡略 <cailue@apache.org>
@coderabbitai
Copy link

coderabbitai bot commented Nov 4, 2025

Warning

Rate limit exceeded

@ClSlaid has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 54 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 3411f99 and f518638.

📒 Files selected for processing (2)
  • tests/data/config/naive/clice.toml (1 hunks)
  • tests/unit/Server/Config.cpp (1 hunks)

Walkthrough

Adds a new naive TOML test configuration and a unit test that parses and validates that configuration (project dirs and three rule blocks with various pattern/append/remove forms).

Changes

Cohort / File(s) Summary
Test Configuration Data
tests/data/config/naive/clice.toml
New TOML file defining a project section (cache_dir, index_dir) and three [[rules]] blocks: one with a C++ pattern and scalar append/remove, one with TypeScript patterns and array append/remove (includes non-ASCII string), and one empty/default rule.
Configuration Parser Test
tests/unit/Server/Config.cpp
New unit test that calls clice::config::Config::parse() for the naive workspace; asserts parse success, workspace name, project directories, presence of three rules, and validates each rule's patterns, append, and remove contents (including checks for empty collections).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify TOML values match the test assertions exactly (paths, strings, arrays).
  • Check parser handling of scalar vs. array entries and empty rule block.
  • Confirm proper handling of non-ASCII content in rule strings.

Poem

🐰
A little toml hopped into place,
Rules stacked neatly in a row;
A test peered in with eager face,
Read every pattern, high and low.
Hooray — the parser learned to know! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title '[test] add workspace loading test' directly matches the PR's purpose of adding a test for config/workspace loading, which aligns with the changes that introduce both test data and a unit test file.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (2)
tests/data/config/naive/clice.toml (2)

8-8: Pattern mismatch with test expectations.

The pattern "../*.cc" doesn't match the test expectation of "**/*.cc" (see test line 30). This inconsistency was already flagged in the test file review.


10-10: Remove value mismatch with test expectations.

The remove value "-std=c++17" doesn't match the test expectation of "-foo" (see test line 34). This inconsistency was already flagged in the test file review.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 39ec9bf and a124dcf.

📒 Files selected for processing (2)
  • tests/data/config/naive/clice.toml (1 hunks)
  • tests/unit/Server/Config.cpp (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: build (windows-2025)
  • GitHub Check: linux (ubuntu-24.04, debug)
  • GitHub Check: macos (macos-15, debug)
  • GitHub Check: linux (ubuntu-24.04, release)
  • GitHub Check: windows (windows-2025)
  • GitHub Check: macos (macos-15, release)
  • GitHub Check: build (macos-15)
  • GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (4)
tests/unit/Server/Config.cpp (2)

9-9: Verify relative path works in all test environments.

The relative path assumes tests are executed from the repository root. Ensure this holds true across local development environments and CI pipelines.


46-48: LGTM! Empty rule verification is correct.

The test correctly verifies that the empty rule block (defined in the TOML as an empty [[rules]] section) has no patterns, append, or remove values.

tests/data/config/naive/clice.toml (2)

1-4: LGTM! Project configuration is well-defined.

The project section correctly defines cache and index directories using the ${workspace} variable substitution pattern.


18-19: LGTM! Empty rule block correctly defined.

The empty [[rules]] block appropriately tests the parser's handling of rules with no configured fields.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
tests/unit/Server/Config.cpp (3)

17-17: Critical: Incorrect workspace expectation (previously flagged).

This issue was already identified in a previous review and remains unaddressed. The test expects workspace == "clice", but Config::parse() assigns the parameter NAIVE_WORKSPACE ("tests/data/config/naive") directly to conf.workspace.

Apply this diff to fix the expectation:

-        expect(that % conf.workspace == "clice");
+        expect(that % conf.workspace == "tests/data/config/naive");

30-30: Critical: Pattern mismatch (previously flagged).

This issue was already identified in a previous review and remains unaddressed. The test expects "**/*.cc", but the TOML configuration defines "../*.cc".

Apply this diff to match the TOML data:

-        expect(that % str_rule.patterns[0] == "**/*.cc");
+        expect(that % str_rule.patterns[0] == "../*.cc");

34-34: Critical: Remove value mismatch (previously flagged).

This issue was already identified in a previous review and remains unaddressed. The test expects "-foo", but the TOML configuration defines "-std=c++17".

Apply this diff to match the TOML data:

-        expect(that % str_rule.remove[0] == "-foo");
+        expect(that % str_rule.remove[0] == "-std=c++17");
🧹 Nitpick comments (1)
tests/unit/Server/Config.cpp (1)

36-48: Consider extracting magic strings to constants.

The test uses many hard-coded string literals for expected values. For improved maintainability and readability, consider extracting them as named constants, especially for the longer values like the Chinese text.

Example refactor:

const std::string EXPECTED_TS_PATTERN = "../*.ts";
const std::string EXPECTED_TSX_PATTERN = "../*.tsx";
const std::string EXPECTED_ES6_TARGET = "--target=es6";
// ... etc

This would make the test more self-documenting and easier to update if the test data changes.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a124dcf and 3411f99.

📒 Files selected for processing (1)
  • tests/unit/Server/Config.cpp (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: windows (windows-2025)
  • GitHub Check: linux (ubuntu-24.04, release)
  • GitHub Check: macos (macos-15, release)
  • GitHub Check: linux (ubuntu-24.04, debug)
  • GitHub Check: build (macos-15)
  • GitHub Check: build (ubuntu-24.04)
  • GitHub Check: build (windows-2025)

@ClSlaid ClSlaid force-pushed the test/config/compile_flags branch 2 times, most recently from 26c084a to 2a52860 Compare November 4, 2025 16:02
Signed-off-by: 蔡略 <cailue@apache.org>
@ClSlaid ClSlaid force-pushed the test/config/compile_flags branch from 2a52860 to f518638 Compare November 4, 2025 16:03
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.

1 participant