test(config): Add unit tests for config utilities; Update test files ESLint config to extend from eslint-config-yscope/jest.
#135
Conversation
…to extend from `eslint-config-yscope/jest`.
WalkthroughThis pull request introduces several changes across multiple files. The Jest configuration in Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/utils/config.ts (2)
106-111: Consider removing unreachable codeSince
testConfigthrows an error for theTHEMEkey, this case in the switch statement is unreachable. Consider removing it to improve code maintainability.- /* c8 ignore start */ - case CONFIG_KEY.THEME: - // Unexpected execution path. - break; - /* c8 ignore end */ /* c8 ignore next */ default: break;
58-60: Consider extracting the error message to a constantThe error message
"${key}" cannot be managed using these utilitiesis duplicated. Consider extracting it to a constant to maintain consistency and ease future updates.+const UNMANAGEABLE_CONFIG_ERROR = (key: string) => + `"${key}" cannot be managed using these utilities.`; // In testConfig - throw new Error(`"${key}" cannot be managed using these utilities.`); + throw new Error(UNMANAGEABLE_CONFIG_ERROR(key)); // In getConfig - throw new Error(`"${key}" cannot be managed using these utilities.`); + throw new Error(UNMANAGEABLE_CONFIG_ERROR(key));Also applies to: 149-151
test/utils/config.test.ts (3)
21-75: Consider centralizing error messages and test constants.While the test constants are well-defined, consider moving error messages and validation constants to a shared constants file to maintain consistency across the codebase.
Consider creating a new file
src/constants/errors.ts:export const CONFIG_ERRORS = { EMPTY_TIMESTAMP_KEY: 'Timestamp key cannot be empty.', EMPTY_LOG_LEVEL_KEY: 'Log level key cannot be empty.', INVALID_PAGE_SIZE: (maxSize: number) => `Page size must be greater than 0 and less than ${maxSize + 1}.`, UNMANAGED_THEME: (key: string) => `"${key}" cannot be managed using these utilities.` } as const;
76-104: Enhance type safety in the test helper function.The helper function could benefit from more explicit type definitions and error handling.
Consider this improvement:
-const runNegativeCases = (func: (input: ConfigUpdate) => Nullable<string>) => { +type TestFunction = (input: ConfigUpdate) => Nullable<string>; +type TestCase = { + input: ConfigUpdate | ConfigUpdate[]; + expected?: Nullable<string | string[]>; + throwable?: Error; +}; + +const runNegativeCases = (func: TestFunction) => { + const runSingleCase = ( + testInput: ConfigUpdate, + expectedResult?: Nullable<string> + ) => { + const result = func(testInput); + expect(result).toBe(expectedResult); + };
105-242: Consider improving test descriptions for clarity.While the test coverage is comprehensive, some test descriptions could be more specific about the scenarios they're testing.
Consider these improvements:
- Line 108: "should return null for valid decoder options" -> "should validate decoder options with all required fields"
- Line 180: "should log an error for invalid page size" -> "should log an error when page size is negative"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
jest.config.ts(1 hunks)package.json(2 hunks)src/utils/config.ts(4 hunks)test/utils/config.test.ts(1 hunks)
🔇 Additional comments (8)
jest.config.ts (2)
34-38: LGTM! Coverage threshold aligns with testing standards.
The addition of 100% coverage threshold for src/utils/config.ts maintains consistency with other utility files like math.ts.
34-38: Verify test coverage implementation.
Let's ensure the new coverage requirements are achievable and properly implemented.
✅ Verification successful
100% test coverage requirement is achievable and properly implemented
The test coverage requirements are reasonable and achievable based on the following evidence:
- The implementation in
config.tsis well-structured with clear, distinct code paths - The test file
config.test.tsdemonstrates comprehensive coverage:- Tests all config operations (get, set, test) for each config key
- Includes negative test cases for invalid inputs
- Verifies error conditions and edge cases
- Tests default values and localStorage interactions
- The only excluded code paths are properly marked with
/* c8 ignore */comments for unreachable defensive code
The existing test suite adequately covers:
- All branches in switch statements
- Validation logic for all config types
- Error handling scenarios
- Default value fallbacks
- Type conversions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of test file and its coverage
# Check if test file exists
echo "Checking for config test file..."
fd "config.test.ts" test/utils/
# Check test coverage implementation
echo "Checking test coverage implementation..."
rg -A 5 "describe|test|it" test/utils/config.test.ts
# Check for any uncovered code markers
echo "Checking for coverage exclusion markers..."
rg "istanbul ignore" src/utils/config.ts
Length of output: 5580
Script:
#!/bin/bash
# Let's check the actual implementation to verify coverage feasibility
# Check the implementation of config.ts
echo "Checking config.ts implementation..."
cat src/utils/config.ts
# Check for any complex logic or edge cases
echo "Checking for complex patterns..."
ast-grep --pattern 'function $FNAME($$$) { $$$ }' src/utils/config.ts
# Check for conditional statements that might affect coverage
echo "Checking conditional statements..."
rg "if|switch|try" src/utils/config.ts
Length of output: 6619
package.json (2)
58-58: LGTM: Jest environment dependency added correctly
The addition of jest-environment-jsdom at version ^29.7.0 aligns perfectly with your existing Jest version and is essential for testing React components in a simulated browser environment.
101-101: LGTM: ESLint configuration simplified
The ESLint configuration for test files has been streamlined by extending from yscope/jest, which is a more maintainable approach than managing environment settings directly.
src/utils/config.ts (3)
58-60: LGTM: Clear and consistent error handling for theme configuration
The error handling for the THEME key is well-implemented and follows the function's documented behaviour.
149-151: LGTM: Consistent error handling pattern
The error handling for the THEME key maintains consistency with the pattern established in testConfig.
177-177: LGTM: Appropriate export of validation constant
Exporting MAX_PAGE_SIZE is appropriate as it represents a validation boundary that consumers might need to reference.
test/utils/config.test.ts (1)
1-20: LGTM! Environment and imports are well-structured.
The test environment and imports are properly configured for testing browser-specific functionality.
test/utils/config.test.ts
Outdated
| it("should retrieve page size once set", () => { | ||
| setConfig({key: CONFIG_KEY.PAGE_SIZE, value: VALID_PAGE_SIZE}); | ||
| const result = getConfig(CONFIG_KEY.PAGE_SIZE); | ||
| expect(result).toBe(VALID_PAGE_SIZE); | ||
| }); |
There was a problem hiding this comment.
Remove duplicate test case.
This test case is a duplicate of the one at lines 219-223.
Remove this duplicate test case:
- it("should retrieve page size once set", () => {
- setConfig({key: CONFIG_KEY.PAGE_SIZE, value: VALID_PAGE_SIZE});
- const result = getConfig(CONFIG_KEY.PAGE_SIZE);
- expect(result).toBe(VALID_PAGE_SIZE);
- });
There was a problem hiding this comment.
I agree with rabbit on this one
There was a problem hiding this comment.
@davemarco, thank you for confirming.
There was a problem hiding this comment.
good catch. fixed now
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
discussed offline with @davemarco and he will review this later in the evening |
| case CONFIG_KEY.THEME: | ||
| // Unexpected execution path. | ||
| break; | ||
| /* c8 ignore end */ |
There was a problem hiding this comment.
Is it better to do this? Or just allow a few lines to be excluded from coverage? Personally i don't know, just want to hear your thoughts
There was a problem hiding this comment.
just allow a few lines to be excluded from coverage?
Do you mean /* c8 ignore next 2 */ for example? I think that can be cleaner indeed
There was a problem hiding this comment.
Oh i was thinking just lowering the coverage to like 99 for this file to avoid this ugly comment.. But ignore this comment for now. If we change the logic for setting the theme, this branch is reachable, and this comment is irrelevant.
There was a problem hiding this comment.
| /** | ||
| * Negative test cases for `testConfig` and `setConfig` function. | ||
| */ | ||
| const NEGATIVE_CONFIG_CASES: Record<string, { |
There was a problem hiding this comment.
Why do this when we could just run them like positive cases?
There was a problem hiding this comment.
That's an attempt to deduplicate the test code for testConfig and setConfig, since everything that fails testConfig would also fail setConfig
There was a problem hiding this comment.
I see. I think the code is a bit hard to read. I suggest two options.
Option 1: Only run all the tests for testConfig() (and write them like normal jest tests), and then just have a single negative test for setConfig(). I think this will still get "100% coverage", but slightly less accurate since we are not testing all errors in setConfig().
Options 2: Try to refactor current code. There may be a cleaner approach something like below where all the tests are defined in a standard jest format, but I have not yet attempted myself.
const defineConfigTests = (func) => {
it('test1', () => {
expect(func(input1).toBe('number');
});
it('test2', () => {
expect(func(input2)).toBeGreaterThan(0);
});
etc...
};
| case CONFIG_KEY.PAGE_SIZE: | ||
| if (0 >= value || MAX_PAGE_SIZE < value) { | ||
| result = `Page size must be greater than 0 and less than ${MAX_PAGE_SIZE + 1}.`; | ||
| } | ||
| break; | ||
| case CONFIG_KEY.THEME: |
There was a problem hiding this comment.
Question why do we not just let user set the theme in local storage? Would this not be simpler? See suggestion in setConfig()
There was a problem hiding this comment.
This was considered an impossible code path (i.e., calling testConfig or setConfig with CONFIG_KEY.THEME at any place would constitute an implementation error) because only JoyUI should have direct access on the theme config. We only created the theme-related types and localStorage keys to avoid type and key conflicts. With that said, i do see that we could be calling setConfig to set the theme once we implement the profile system, where we could be restoring theme name settings from a profile.
If you agree, we can implement CONFIG_KEY.THEME in the config utilities in preparation of the config profile system.
| case CONFIG_KEY.PAGE_SIZE: | ||
| if (0 >= value || MAX_PAGE_SIZE < value) { | ||
| result = `Page size must be greater than 0 and less than ${MAX_PAGE_SIZE + 1}.`; | ||
| } | ||
| break; | ||
| case CONFIG_KEY.THEME: | ||
| throw new Error(`"${key}" cannot be managed using these utilities.`); | ||
| /* c8 ignore next */ |
There was a problem hiding this comment.
You can actually remove this and will still get 100% coverage
| /* c8 ignore next */ |
There was a problem hiding this comment.
That seems to be required to achieve 100% branch coverage. With this exclusion removed, do you see below error when you run npm run test?
Jest: "src/utils/config.ts" coverage threshold for branches (100%) not met: 97.14%
...
Ran all test suites.
Process finished with exit code 1
| case CONFIG_KEY.THEME: | ||
| // Unexpected execution path. | ||
| break; | ||
| /* c8 ignore end */ |
There was a problem hiding this comment.
Oh i was thinking just lowering the coverage to like 99 for this file to avoid this ugly comment.. But ignore this comment for now. If we change the logic for setting the theme, this branch is reachable, and this comment is irrelevant.
| // Unexpected execution path. | ||
| break; | ||
| /* c8 ignore end */ | ||
| /* c8 ignore next */ |
There was a problem hiding this comment.
You can remove this and still get 100% coverage
| /* c8 ignore next */ |
| case CONFIG_KEY.PAGE_SIZE: | ||
| value = window.localStorage.getItem(LOCAL_STORAGE_KEY.PAGE_SIZE); | ||
| break; | ||
| case CONFIG_KEY.THEME: | ||
| throw new Error(`"${key}" cannot be managed using these utilities.`); | ||
| /* c8 ignore next */ |
There was a problem hiding this comment.
| /* c8 ignore next */ |
| } from "../../src/utils/config"; | ||
|
|
||
|
|
||
| const VALID_DECODER_OPTIONS: DecoderOptions = { |
There was a problem hiding this comment.
I think you should add brief doc strings here like
| const VALID_DECODER_OPTIONS: DecoderOptions = { | |
| /** | |
| * Sample decoder options that are expected to pass validation. | |
| */ | |
| const VALID_DECODER_OPTIONS: DecoderOptions = { |
Same for definitions below.
| /** | ||
| * Negative test cases for `testConfig` and `setConfig` function. | ||
| */ | ||
| const NEGATIVE_CONFIG_CASES: Record<string, { |
There was a problem hiding this comment.
I see. I think the code is a bit hard to read. I suggest two options.
Option 1: Only run all the tests for testConfig() (and write them like normal jest tests), and then just have a single negative test for setConfig(). I think this will still get "100% coverage", but slightly less accurate since we are not testing all errors in setConfig().
Options 2: Try to refactor current code. There may be a cleaner approach something like below where all the tests are defined in a standard jest format, but I have not yet attempted myself.
const defineConfigTests = (func) => {
it('test1', () => {
expect(func(input1).toBe('number');
});
it('test2', () => {
expect(func(input2)).toBeGreaterThan(0);
});
etc...
};
| }); | ||
|
|
||
| it("should log an error for invalid page size", () => { | ||
| const consoleSpy = jest.spyOn(console, "error").mockImplementation(); |
There was a problem hiding this comment.
What is the point of this mock? Does it do anything? If not I would remove and just make it simpler.
There was a problem hiding this comment.
Like is the
expect(result).toBe(
`Page size must be greater than 0 and less than ${MAX_PAGE_SIZE + 1}.` );
enough by itself for this unit test?
There was a problem hiding this comment.
My mistake here. The mockImplementation() call should be called with a dummy jest.fn() (i.e., it should be jest.spyOn(console, "error").mockImplementation(jest.fn()) so that the error message doesn't get printed in the console. When it does show, it shows up with a stack trace in jest's output. I mistakenly thought that argument was fine to be removed when I ran the tests with PyCharm.
Like is the ... enough by itself for this unit test?
Sure, if we treat those as debug messages that only concern us developers (sounds right to me). Do you think it would be a good idea to check expect(consoleSpy).toHaveBeenCalled() or expect(consoleSpy).toHaveBeenCalledTimes(1) so that we are not blindly suppressing the console errors? That way we can be notified about such expectation changes when our implementation accidentally / intentionally removes a console error log.
test/utils/config.test.ts
Outdated
| it("should retrieve page size once set", () => { | ||
| setConfig({key: CONFIG_KEY.PAGE_SIZE, value: VALID_PAGE_SIZE}); | ||
| const result = getConfig(CONFIG_KEY.PAGE_SIZE); | ||
| expect(result).toBe(VALID_PAGE_SIZE); | ||
| }); |
There was a problem hiding this comment.
I agree with rabbit on this one
| case CONFIG_KEY.PAGE_SIZE: | ||
| window.localStorage.setItem(LOCAL_STORAGE_KEY.PAGE_SIZE, value.toString()); | ||
| break; | ||
| /* c8 ignore start */ | ||
| case CONFIG_KEY.THEME: | ||
| // Unexpected execution path. |
There was a problem hiding this comment.
| // Unexpected execution path. | |
| window.localStorage.setItem(LOCAL_STORAGE_KEY.THEME, value.toString()); |
There was a problem hiding this comment.
Why dont we save the theme selection to local storage?
There was a problem hiding this comment.
Discussed at https://github.com/y-scope/yscope-log-viewer/pull/135/files#r1876734225 : we allocate the types and enums for theme config only to avoid name conflicts and we expected only JoyUI to read / write into localStorage, which might subject to change in the future.
Description
jest-environment-jsdom.eslint-config-yscope/jest.src/utils/config.ts.src/utils/config.ts.Validation performed
Summary by CodeRabbit
New Features
MAX_PAGE_SIZE, for configuration management.Bug Fixes
Documentation
Chores