Skip to content

test(linter): Add more tests for deserializing globals in Oxlintrc.#16858

Merged
graphite-app[bot] merged 1 commit intomainfrom
add-deserialization-tests-for-globals
Dec 15, 2025
Merged

test(linter): Add more tests for deserializing globals in Oxlintrc.#16858
graphite-app[bot] merged 1 commit intomainfrom
add-deserialization-tests-for-globals

Conversation

@connorshea
Copy link
Member

Just ensuring they work correctly by checking that they get mapped to the correct enum variants.

  • readonly/readable/false should map to Readonly
  • writable/writeable/true should map to Writable
  • off should map to Off

@connorshea connorshea requested a review from camc314 as a code owner December 14, 2025 20:47
Copilot AI review requested due to automatic review settings December 14, 2025 20:47
@github-actions github-actions bot added A-linter Area - Linter C-test Category - Testing. Code is missing test cases, or a PR is adding them labels Dec 14, 2025
Copy link

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

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

The tests generally improve coverage of globals deserialization, but one assertion is incorrect: test_deserialize_legacy_spelling currently expects "off" to be enabled. Also, globals.rs tests directly access globals.0, coupling tests to internal representation; prefer a public accessor (consistent with mod.rs). Finally, the new test_deserialize_globals could better align with the PR intent by asserting is_enabled for an off entry.

Additional notes (1)
  • Maintainability | crates/oxc_linter/src/config/mod.rs:111-136
    This new test validates variant mapping but doesn't assert the enabled/disabled behavior for entries like "baz": "off". Given the PR intent (and the existing tests in globals.rs), it's worth also asserting is_enabled for at least one off entry to ensure behavior and representation stay aligned.
Summary of changes

What changed

✅ Expanded coverage for globals deserialization

  • Added assertions in crates/oxc_linter/src/config/globals.rs tests to verify string/boolean inputs map to the correct GlobalValue enum variants (e.g., "readonly" -> Readonly, true -> Writable, "off" -> Off).
  • Added a new test_deserialize_globals in crates/oxc_linter/src/config/mod.rs to ensure Oxlintrc config parsing correctly deserializes multiple globals values (including legacy spellings and booleans) into the expected enum variants.

🔎 Stronger guarantees

  • Tests now validate both behavior (is_enabled) and the underlying stored representation (get(...) / direct map access).

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds comprehensive test coverage for deserializing global variable configurations in Oxlintrc. It verifies that all supported string aliases (readable, writeable, readonly, writable, off) and boolean values (true, false) correctly map to their corresponding GlobalValue enum variants (Readonly, Writable, Off).

  • Adds a new comprehensive test test_deserialize_globals in the main config module that tests all supported global value formats
  • Enhances existing tests in globals.rs to verify the correct enum variant mapping, not just enabled/disabled state

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/oxc_linter/src/config/mod.rs Adds new comprehensive test for global deserialization covering all aliases and boolean values, plus additional assertion to existing test
crates/oxc_linter/src/config/globals.rs Enhances three existing tests with assertions that verify the correct GlobalValue enum variant is assigned

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 14, 2025

CodSpeed Performance Report

Merging #16858 will not alter performance

Comparing add-deserialization-tests-for-globals (3325dd7) with main (d402242)

Summary

✅ 4 untouched
⏩ 41 skipped1

Footnotes

  1. 41 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@camc314 camc314 self-assigned this Dec 15, 2025
@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Dec 15, 2025
Copy link
Contributor

camc314 commented Dec 15, 2025

Merge activity

…16858)

Just ensuring they work correctly by checking that they get mapped to the correct enum variants.

- `readonly`/`readable`/`false` should map to Readonly
- `writable`/`writeable`/`true` should map to Writable
- `off` should map to Off
@graphite-app graphite-app bot force-pushed the add-deserialization-tests-for-globals branch from 3325dd7 to 08f4014 Compare December 15, 2025 09:50
@graphite-app graphite-app bot merged commit 08f4014 into main Dec 15, 2025
20 checks passed
@graphite-app graphite-app bot deleted the add-deserialization-tests-for-globals branch December 15, 2025 09:55
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter C-test Category - Testing. Code is missing test cases, or a PR is adding them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants