Repository navigation
feat:minor key changes for tags - #5981
openshift-merge-bot[bot] merged 1 commit into
Conversation
📝 WalkthroughWalkthroughThis pull request renames the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/feature-store/src/components/FeatureStoreGlobalSearch/utils/__tests__/searchUtils.spec.ts (1)
467-543: Update test descriptions and comments to reflect the renamed property.The test descriptions and inline comments still reference
matched_tag(singular), but the property has been renamed tomatched_tags(plural). Update these for consistency:
- Line 467:
"should preserve matched_tags property when grouping items"- Line 506:
// Verify matched_tags is preserved for first item- Line 511:
// Verify matched_tags is preserved for second item- Line 519:
// Verify item without matched_tags doesn't have it- Line 525:
"should handle items with empty matched_tags object"📝 Suggested updates
- it('should preserve matched_tag property when grouping items', () => { + it('should preserve matched_tags property when grouping items', () => {- // Verify matched_tag is preserved for first item + // Verify matched_tags is preserved for first item- // Verify matched_tag is preserved for second item + // Verify matched_tags is preserved for second item- // Verify item without matched_tag doesn't have it + // Verify item without matched_tags doesn't have it- it('should handle items with empty matched_tag object', () => { + it('should handle items with empty matched_tags object', () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.tspackages/feature-store/src/__mocks__/mockGlobalSearch.tspackages/feature-store/src/apiHooks/useFeatureStoreSearch.tspackages/feature-store/src/components/FeatureStoreGlobalSearch/GlobalSearchInput.tsxpackages/feature-store/src/components/FeatureStoreGlobalSearch/hooks/useSearchHandlers.tspackages/feature-store/src/components/FeatureStoreGlobalSearch/utils/__tests__/searchUtils.spec.tspackages/feature-store/src/types/search.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/cypress-e2e.mdc)
**/*.{ts,tsx}: Run linting from the packages/cypress directory withnpm run lint -- --fixbefore claiming test is complete - all linting errors must be fixed
Use object destructuring for variables, proper formatting and spacing, no unused variables or imports, avoidanytypes unless absolutely necessary, and follow existing code patterns
Files:
packages/feature-store/src/components/FeatureStoreGlobalSearch/hooks/useSearchHandlers.tspackages/feature-store/src/apiHooks/useFeatureStoreSearch.tspackages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.tspackages/feature-store/src/components/FeatureStoreGlobalSearch/GlobalSearchInput.tsxpackages/feature-store/src/components/FeatureStoreGlobalSearch/utils/__tests__/searchUtils.spec.tspackages/feature-store/src/types/search.tspackages/feature-store/src/__mocks__/mockGlobalSearch.ts
**/*.{cy.ts,ts}
📄 CodeRabbit inference engine (.cursor/rules/cypress-e2e.mdc)
Never hardcode namespaces in tests or utilities - always derive namespaces from test variables using
Cypress.env('APPLICATIONS_NAMESPACE')or similar environment variables loaded fromtest-variables.yml
Files:
packages/feature-store/src/components/FeatureStoreGlobalSearch/hooks/useSearchHandlers.tspackages/feature-store/src/apiHooks/useFeatureStoreSearch.tspackages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.tspackages/feature-store/src/components/FeatureStoreGlobalSearch/utils/__tests__/searchUtils.spec.tspackages/feature-store/src/types/search.tspackages/feature-store/src/__mocks__/mockGlobalSearch.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/cypress-mock.mdc)
Use
data-testidattributes with kebab-case naming (e.g.,message-input,delete-button,user-settings-form) on all interactive elements, buttons, inputs, forms, modals, and key UI components
Files:
packages/feature-store/src/components/FeatureStoreGlobalSearch/hooks/useSearchHandlers.tspackages/feature-store/src/apiHooks/useFeatureStoreSearch.tspackages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.tspackages/feature-store/src/components/FeatureStoreGlobalSearch/GlobalSearchInput.tsxpackages/feature-store/src/components/FeatureStoreGlobalSearch/utils/__tests__/searchUtils.spec.tspackages/feature-store/src/types/search.tspackages/feature-store/src/__mocks__/mockGlobalSearch.ts
**/*.{ts,tsx,cy.ts}
📄 CodeRabbit inference engine (.cursor/rules/cypress-mock.mdc)
Use selector priority in order: (1)
data-testid(REQUIRED), (2)findByLabelText/findByRole(only if testID cannot be added), (3) Text content (only for non-interactive elements), NEVER use DOM structure, CSS classes, or placeholders
Files:
packages/feature-store/src/components/FeatureStoreGlobalSearch/hooks/useSearchHandlers.tspackages/feature-store/src/apiHooks/useFeatureStoreSearch.tspackages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.tspackages/feature-store/src/components/FeatureStoreGlobalSearch/GlobalSearchInput.tsxpackages/feature-store/src/components/FeatureStoreGlobalSearch/utils/__tests__/searchUtils.spec.tspackages/feature-store/src/types/search.tspackages/feature-store/src/__mocks__/mockGlobalSearch.ts
**/*.cy.ts
📄 CodeRabbit inference engine (.cursor/rules/cypress-e2e.mdc)
**/*.cy.ts: Load test variables usingcy.getTestConfig().then((config) => { ... })pattern
Load fixtures usingcy.fixture('e2e/path/file.yaml')pattern
Use descriptive test file names matching feature area, e.g.,testFeatureName.cy.tsfor main functionality
Specify tags in the test file within theit()block options, e.g.,it('...', { tags: ['@Tag'] }, ...)- never in fixture files
Never usecy.findByTestId()directly in tests - all UI interactions must go through page objects
Never usecy.findByRole()directly in tests - all UI interactions must go through page objects
Do not callcy.get()directly in test files; use it only inside page-objectfind...helper methods
All UI interactions must go through page objects - if a test ID exists but no page object, create the page object method; if test ID doesn't exist, create both the test ID and page object method
Use.navigate()methods when available instead ofcy.visit()for navigation
Usecy.step('Description')instead ofcy.log()for test documentation - steps auto-number and provide better test reporting
Never usecy.wait(milliseconds)for arbitrary time periods - use OC commands to wait for resource state changes or wait for specific UI elements with proper selectors
Use OC commands to wait for resource readiness, e.g.,cy.exec('oc wait --for=condition=Ready pod/my-pod --timeout=300s')
Prefer test ID validation over text validation - usedata-testidattributes for validation when possible; avoid text-based assertions unless absolutely necessary
Add[Product Bug: JIRA-XXXXX]prefix to describe block when quarantining a test due to a product bug, e.g.,describe('[Product Bug: RHOAIENG-12345] Feature Name', () => { ... })
Add[Automation Bug: JIRA-XXXXX]prefix to describe block when quarantining a test due to an automation issue, e.g.,describe('[Automation Bug: RHOAIENG-12345] Feature Name', () => { ... })
Add@Bugtag to the tags array when quarantining a test due to a pr...
Files:
packages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.ts
packages/cypress/cypress/tests/mocked/**/*.cy.ts
📄 CodeRabbit inference engine (.cursor/rules/cypress-mock.mdc)
packages/cypress/cypress/tests/mocked/**/*.cy.ts: Organize mock tests inpackages/cypress/cypress/tests/mocked/directory with file naming likefeatureName.cy.tsand one file per page or feature area
Use MANDATORYdescribe/itstructure withbeforeEachfor common setup. Avoid using tags orcy.step()in mock tests (exception: GenAI module uses tags andcy.step()per team decision)
Always mock ALL network requests - usecy.interceptOdh()for Dashboard API endpoints,cy.interceptK8s()for single Kubernetes resources,cy.interceptK8sList()for Kubernetes lists. Never make real network requests in mock tests.
Use properdescribe/itstructure with specific validation in wait methods. Always validate specific text content in wait methods, not just existence:cy.findByTestId('app-page-title').should('have.text', 'User management')
Validate actual request payloads in interceptors using JSONPath operations:expect(interception.request.body).to.eql([{ value: [...], op: 'replace', path: '...' }])
Test access control by creating separate tests: one outside describe block with non-admin users, and a describe block with admin-only tests. Use appropriate user mocks:asProductAdminUser(),asProjectAdminUser(),asProjectEditUser()
Test feature flags by mocking dashboard config with disabled flags:mockDashboardConfig({ disableFeatureName: true })
Test error handling by mocking failed API responses and validating error messages are displayed to users
Include accessibility testing in all tests by callingcy.testA11y()in page object wait methods and after page state changes
Apply minimal mocking principle: only mock endpoints necessary for the test. Don't over-mock endpoints that aren't used.
Never use tags orcy.step()in mock tests for general dashboard features. Exception: GenAI module uses tags andcy.step()in mock tests (team decision).
For table sorting tests, use helper utilities: `table.findTableHeaderButton('Name').should(be.sortAscendin...
Files:
packages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.ts
packages/cypress/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/cypress-mock.mdc)
Always run linting before claiming test is complete:
cd packages/cypress && npm run lint -- --fix. Fix ALL linting errors including object destructuring, formatting, unused variables, and proper TypeScript types.
Files:
packages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.ts
packages/cypress/cypress/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/cypress-mock.mdc)
Create test helper functions for repeated test patterns. Use
initIntercepts()pattern to consolidate multiplecy.intercept*calls into single function for reusability across tests.
Files:
packages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.ts
🧠 Learnings (5)
📚 Learning: 2025-12-09T21:53:53.864Z
Learnt from: DaoDaoNoCode
Repo: opendatahub-io/odh-dashboard PR: 5521
File: packages/kserve/src/hardware.ts:1-33
Timestamp: 2025-12-09T21:53:53.864Z
Learning: Guideline: In TypeScript, you can use import type { foo } and reference it as Parameters<typeof foo> or ReturnType<typeof foo> in type-only positions (e.g., return types, type aliases). This compiles when typeof foo is used exclusively in type positions and the runtime code does not reference the imported symbol. Use this to avoid pulling runtime values into emitted code while preserving function type information. Apply to all TypeScript files (/**/*.ts) where function types are needed for type utilities.
Applied to files:
packages/feature-store/src/components/FeatureStoreGlobalSearch/hooks/useSearchHandlers.tspackages/feature-store/src/apiHooks/useFeatureStoreSearch.tspackages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.tspackages/feature-store/src/components/FeatureStoreGlobalSearch/utils/__tests__/searchUtils.spec.tspackages/feature-store/src/types/search.tspackages/feature-store/src/__mocks__/mockGlobalSearch.ts
📚 Learning: 2026-01-09T09:09:00.525Z
Learnt from: ChughShilpa
Repo: opendatahub-io/odh-dashboard PR: 5898
File: packages/cypress/cypress/tests/e2e/modelTraining/testTrainjobProgression.cy.ts:112-179
Timestamp: 2026-01-09T09:09:00.525Z
Learning: In Cypress tests under packages/cypress, use cy.log() for conditional informational messages (such as skip notices) and use cy.step() to document actual test steps and actions. This distinguishes setup/conditions from the steps that define test execution, improving readability and traceability of test flows across the Cypress test suite.
Applied to files:
packages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.ts
📚 Learning: 2026-01-09T21:21:06.041Z
Learnt from: nananosirova
Repo: opendatahub-io/odh-dashboard PR: 5631
File: packages/cypress/cypress/pages/pipelines/mlflowExperiments.ts:3-4
Timestamp: 2026-01-09T21:21:06.041Z
Learning: Enforce architectural boundaries in Cypress test packages by disallowing imports from frontend/source code (e.g., frontend/src) in tests under packages/cypress. This includes constants; use shared test utilities or mock data instead. This restriction is enforced by ESLint rules (no-restricted-syntax) to prevent triggering restricted syntax patterns and to maintain separation between Cypress tests and frontend implementation. Apply this pattern to all TypeScript files under packages/cypress (e.g., packages/cypress/**/*.ts, and extend to ts/tsx as needed).
Applied to files:
packages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.ts
📚 Learning: 2026-01-09T21:22:33.830Z
Learnt from: nananosirova
Repo: opendatahub-io/odh-dashboard PR: 5631
File: packages/cypress/cypress/tests/mocked/pipelines/mlflowExperiments.cy.ts:6-9
Timestamp: 2026-01-09T21:22:33.830Z
Learning: In the odh-dashboard repository, Cypress tests under packages/cypress must not import constants or implementation code from frontend/src due to eslint no-restricted-syntax rules that enforce test isolation. If a value is needed in tests, duplicate simple constants within the test file instead of importing from frontend. This preserves test isolation and prevents cross-package coupling.
Applied to files:
packages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.ts
📚 Learning: 2026-01-09T21:22:40.332Z
Learnt from: nananosirova
Repo: opendatahub-io/odh-dashboard PR: 5631
File: packages/cypress/cypress/pages/pipelines/mlflowExperiments.ts:3-4
Timestamp: 2026-01-09T21:22:40.332Z
Learning: In the odh-dashboard repository, Cypress tests under packages/cypress must not import frontend code (e.g., odh-dashboard/internal or frontend/src). Enforce this with eslint no-restricted-syntax rules to maintain architectural boundaries. If a constant is needed in tests, duplicate a simple constant instead of importing or disabling the rule; prefer maintainability and avoid cross-layer imports.
Applied to files:
packages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.ts
⏰ 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). (4)
- GitHub Check: Type-Check
- GitHub Check: Get-Test-Groups
- GitHub Check: Lint
- GitHub Check: Cypress-Setup
🔇 Additional comments (9)
packages/feature-store/src/types/search.ts (1)
3-11: LGTM!The type rename from
matched_tagtomatched_tagsis semantically correct—theRecord<string, string>type inherently supports multiple key-value pairs, and the plural naming better reflects this capability. The change aligns with the upstream Feast API changes.packages/feature-store/src/components/FeatureStoreGlobalSearch/hooks/useSearchHandlers.ts (1)
6-15: LGTM!The
ISearchIteminterface correctly mirrors theGlobalSearchResulttype rename. The property change is consistent with the core type definition.packages/feature-store/src/components/FeatureStoreGlobalSearch/GlobalSearchInput.tsx (1)
286-307: LGTM!The property access is correctly updated to use
matched_tags. The conditional rendering and iteration logic remain sound.Note: The
data-testidattributeglobal-search-matched-tag-${key}uses singular "tag" which is appropriate since each rendered label represents a single key-value pair.packages/cypress/cypress/tests/mocked/featureStore/featureEntities.cy.ts (2)
635-666: LGTM!The test correctly verifies single matched tag display. The mock data modification at line 643 properly uses the renamed
matched_tagsproperty, and the assertions validate both visibility and content.
668-709: LGTM!The test for multiple matched tags is well-structured—it covers displaying several tags simultaneously and validates each one individually. The test data at line 676-680 demonstrates proper usage of the
matched_tagsobject with multiple entries.packages/feature-store/src/__mocks__/mockGlobalSearch.ts (1)
90-109: LGTM!The
transformToSearchItemsfunction correctly updates both the return type annotation (line 99) and the property mapping (line 108) to usematched_tags. The pass-through of the optional property is appropriate—tests can injectmatched_tagsvia thepartialparameter when needed.packages/feature-store/src/apiHooks/useFeatureStoreSearch.ts (1)
16-16: LGTM! Property rename is consistent with the Feast API changes.The rename from
matched_tagtomatched_tagsis correctly applied in both the return type definition and the data transformation logic. The ESLint disable comment is appropriate since the property name follows the API's snake_case convention.Also applies to: 60-61
packages/feature-store/src/components/FeatureStoreGlobalSearch/utils/__tests__/searchUtils.spec.ts (2)
89-108: LGTM! Test helper updated correctly.The
createMockSearchItemhelper function is properly updated to usematched_tagsin both the parameter name and the returned object property.
506-542: LGTM! Test assertions correctly updated.The assertions properly verify the
matched_tagsproperty, including cases for:
- Single tag object
- Multiple tags object
- Undefined (when not provided)
- Empty object
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5981 +/- ##
=======================================
Coverage 64.37% 64.37%
=======================================
Files 2389 2389
Lines 71176 71176
Branches 17819 17819
=======================================
+ Hits 45820 45823 +3
+ Misses 25356 25353 -3
... and 19 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dpanshug, manaswinidas The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
505c7a4
into
opendatahub-io:main
3 participants
Description
This PR contains minor key changes from matched_tag to matched_tags wrt Feast global search changes
with support for single and multiple tags
How Has This Been Tested?
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main