CONSOLE-5286 Block snapshot tests via ESLint and migrate existing tests to explicit assertions#16427
CONSOLE-5286 Block snapshot tests via ESLint and migrate existing tests to explicit assertions#16427cajieh wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
component/dev-console
cajieh
force-pushed
the
remove-and-block-snapshot-tests
branch
2 times, most recently
from
0d75c81 to
8f2049a
Compare
cajieh
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (6)frontend/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (STYLEGUIDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (STYLEGUIDE.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (STYLEGUIDE.md)
Files:
**/*.spec.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (STYLEGUIDE.md)
Files:
frontend/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (README.md)
Files:
🔇 Additional comments (5)
📝 WalkthroughWalkthroughUpdated RTL testing standards to explicitly prohibit 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/packages/topology/src/data-transforms/__tests__/data-transformer.spec.ts (1)
96-103: ⚡ Quick winAvoid hard-coded UID literals in expected edge payload.
Line 98-Line 101 couple this test to fixture UUID constants. Derive
source/target/idfrommockResourcesso fixture refreshes don’t cause noisy, non-behavioral failures.Proposed refactor
- expect(graphData.edges).toStrictEqual([ - { - id: '5ca9ae28-680d-11e9-8c69-5254003f9382_60a9b423-680d-11e9-8c69-5254003f9382', - label: 'Visual connector', - source: '5ca9ae28-680d-11e9-8c69-5254003f9382', - target: '60a9b423-680d-11e9-8c69-5254003f9382', - type: 'connects-to', - }, - ]); + const sourceUid = mockResources.deployments.data[0].metadata.uid; + const targetUid = mockResources.deployments.data[1].metadata.uid; + expect(graphData.edges).toStrictEqual([ + { + id: `${sourceUid}_${targetUid}`, + label: 'Visual connector', + source: sourceUid, + target: targetUid, + type: 'connects-to', + }, + ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/topology/src/data-transforms/__tests__/data-transformer.spec.ts` around lines 96 - 103, The test currently asserts graphData.edges against hard-coded UUIDs; change it to derive source, target and edge id from the test fixtures (mockResources) so it stays stable when fixture UUIDs change: compute sourceId and targetId from the corresponding mockResources entries (e.g. mockResources[index].uid) and assert that graphData.edges contains an object with id `${sourceId}_${targetId}`, source: sourceId, target: targetId, label: 'Visual connector' and type: 'connects-to' instead of using the literal UUID strings; update the expectation in the test (the assertion that references graphData.edges) to use these derived values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@frontend/packages/topology/src/data-transforms/__tests__/data-transformer.spec.ts`:
- Around line 96-103: The test currently asserts graphData.edges against
hard-coded UUIDs; change it to derive source, target and edge id from the test
fixtures (mockResources) so it stays stable when fixture UUIDs change: compute
sourceId and targetId from the corresponding mockResources entries (e.g.
mockResources[index].uid) and assert that graphData.edges contains an object
with id `${sourceId}_${targetId}`, source: sourceId, target: targetId, label:
'Visual connector' and type: 'connects-to' instead of using the literal UUID
strings; update the expectation in the test (the assertion that references
graphData.edges) to use these derived values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 239e570b-fff9-4adc-8f2f-dc1d207d7a2b
⛔ Files ignored due to path filters (2)
frontend/packages/dev-console/src/utils/__tests__/__snapshots__/shared-submit-utils.spec.ts.snapis excluded by!**/*.snapfrontend/packages/topology/src/data-transforms/__tests__/__snapshots__/data-transformer.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
.claude/skills/gen-rtl-test/SKILL.mdfrontend/packages/dev-console/src/utils/__tests__/shared-submit-utils.spec.tsfrontend/packages/eslint-plugin-console/lib/config/rules/jest.jsfrontend/packages/eslint-plugin-console/lib/config/testing-library-tests.jsfrontend/packages/topology/src/data-transforms/__tests__/data-transformer.spec.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: When writing code for static plugins, ensure that all$codeRefALWAYS reference the corresponding extension type from the dynamic plugin SDK package (@console/dynamic-plugin-sdk) for type safety and consistency across both static and dynamic plugins
Never import from package index files (e.g.,@console/shared) in new code; import from specific file paths instead to avoid circular dependencies and slow builds
Never import from deprecated packages or use code which has the@deprecatedTSdoc tag in new code
Never use template literals (backticks) with translatable strings in the i18nt()function; use single or double quotes instead, as the i18n parser cannot extract keys from template literals
Never use absolute URLs or paths in code; the console runs behind a proxy under an arbitrary path and must use relative paths instead
**/*.{ts,tsx}: Prefer functional programming patterns and immutable data structures in TypeScript/JavaScript
Check existing types inconsole-sharedbefore creating new types
Use PascalCase for component files, kebab-case for utilities, and*.spec.ts(x)for test files
Flag use ofanytype in TypeScript and suggest proper type definitions
Verify that null/undefined are properly handled in TypeScript with union types (e.g.,string | undefined)
Reuse type definitions from existing components rather than duplicating them (e.g., React.ComponentProps<>)
Avoid deprecated components: check for@deprecatedJSDoc tags, /deprecated import paths, or DEPRECATED_ file name prefix
Use direct imports to specific files instead of barrel exports (index.ts) to avoid circular dependencies and improve build performance
Useimport typefor type-only imports in TypeScript
**/*.{ts,tsx}: Don't use backticks inside of aTFunctioncall - use regular quotes instead. The code parser will not automatically pick up keys that contain backticks. Uset('key')instead oft(key).
Any usage of i18next'sTFunction(rather than react...
Files:
frontend/packages/topology/src/data-transforms/__tests__/data-transformer.spec.tsfrontend/packages/dev-console/src/utils/__tests__/shared-submit-utils.spec.ts
**/*.{test.ts,test.tsx,spec.ts,spec.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Consult TESTING.md for testing frameworks, patterns, and best practices before writing or modifying tests; use React Testing Library patterns for component tests
Files:
frontend/packages/topology/src/data-transforms/__tests__/data-transformer.spec.tsfrontend/packages/dev-console/src/utils/__tests__/shared-submit-utils.spec.ts
**/*
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Use lowercase dash-separated names for all files to avoid git issues with case-insensitive file systems
Files:
frontend/packages/topology/src/data-transforms/__tests__/data-transformer.spec.tsfrontend/packages/dev-console/src/utils/__tests__/shared-submit-utils.spec.tsfrontend/packages/eslint-plugin-console/lib/config/rules/jest.jsfrontend/packages/eslint-plugin-console/lib/config/testing-library-tests.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx,js,jsx}: New code MUST be written in TypeScript, not JavaScript
Run the linter and follow all rules defined in .eslintrc
Never use absolute paths in code; the app should be able to run behind a proxy under an arbitrary path
Files:
frontend/packages/topology/src/data-transforms/__tests__/data-transformer.spec.tsfrontend/packages/dev-console/src/utils/__tests__/shared-submit-utils.spec.tsfrontend/packages/eslint-plugin-console/lib/config/rules/jest.jsfrontend/packages/eslint-plugin-console/lib/config/testing-library-tests.js
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
TypeScript/JavaScript tests should follow a table-driven test convention similar to Go tests where applicable
Files:
frontend/packages/topology/src/data-transforms/__tests__/data-transformer.spec.tsfrontend/packages/dev-console/src/utils/__tests__/shared-submit-utils.spec.ts
🔇 Additional comments (4)
frontend/packages/eslint-plugin-console/lib/config/rules/jest.js (1)
68-80: Good enforcement point for snapshot matcher blocking.Centralizing
jest/no-restricted-matchershere with explicit guidance messages is a clean rollout path and keeps policy discoverable.frontend/packages/eslint-plugin-console/lib/config/testing-library-tests.js (1)
20-36: Scoped Jest matcher restriction integration looks solid.Including the Jest plugin/env in this override and reusing the shared rule config keeps enforcement consistent across RTL test globs.
.claude/skills/gen-rtl-test/SKILL.md (1)
623-625: Policy update is clear and aligned with enforcement.This makes the snapshot ban explicit and ties it to the actual lint gate, which helps avoid ambiguity for future test authoring.
frontend/packages/dev-console/src/utils/__tests__/shared-submit-utils.spec.ts (1)
25-94: Strong migration from snapshots to explicit API-object assertions.These
toStrictEqualchecks are concrete and make regressions in generated Service/Route payloads much easier to diagnose.Also applies to: 184-260
cajieh
force-pushed
the
remove-and-block-snapshot-tests
branch
from
8f2049a to
e49acf3
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Tech-debt: |
openshift-ci
Bot
added
px-approved
| 'jest/no-jasmine-globals': 'error', | ||
| // Limited snapshot sizes to keep snapshops manageable and reviewable. | ||
| // Limited snapshot sizes to keep snapshots manageable and reviewable. | ||
| 'jest/no-large-snapshots': 'off', |
There was a problem hiding this comment.
nit: aren't we actually banning all snapshots?
|
/lgtm |
|
/verified by @stefanonardo |
openshift-ci-robot
added
the
verified
|
@stefanonardo: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
cajieh
force-pushed
the
remove-and-block-snapshot-tests
branch
from
e49acf3 to
096002f
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cajieh, stefanonardo The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
//retest |
|
/test all |
|
/retest-required |
|
@cajieh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
CONSOLE-5286 Block snapshot tests via ESLint and migrate existing tests to explicit assertions
Analysis / Root cause:
Snapshot tests are brittle, provide false confidence, and test implementation details rather than user behavior. The codebase has 5 snapshot test files that make tests difficult to maintain and review. Per modern testing practices, explicit assertions should be used instead.
Solution description:
jest/no-restricted-matchersESLint rule to "eslint-plugin-console" that errors on:- toMatchSnapshot
- toMatchInlineSnapshot
- toThrowErrorMatchingSnapshot
- toThrowErrorMatchingInlineSnapshot
- shared-submit-utils.spec.ts: Replaced 4 snapshot tests with toStrictEqual assertions on Service and Route objects
- data-transformer.spec.ts: Replaced 1 snapshot test (2500+ lines) with focused assertions on node count, edge structure, and group presence
Screenshots / screen recording:
N/A - No visual changes. This is a testing infrastructure change.
Test setup:
No special setup required. Run yarn test and yarn lint in the frontend directory.
Test cases:
Console Approver:
/assign
Docs approver:
/assign
PX approver:
/assign
-->
Summary by CodeRabbit
Documentation
Tests
Chores