Skip to content

Re-enable the filters (auto-complete) tests#461

Open
jlpadilla wants to merge 5 commits into
mainfrom
jorge/enable-filter-tests
Open

Re-enable the filters (auto-complete) tests#461
jlpadilla wants to merge 5 commits into
mainfrom
jorge/enable-filter-tests

Conversation

@jlpadilla
Copy link
Copy Markdown
Contributor

@jlpadilla jlpadilla commented May 5, 2026

Summary by CodeRabbit

  • Tests
    • Expanded filter validation to cover operator-prefixed comparisons (<=, >=, !=, =, <, >) and relative date keywords (hour/day/week/month/year).
    • Added numeric and boolean-aware matching, plus substring/array semantics for non-numeric values.
    • Replaced todos with active assertions that build queries, execute requests, and verify filter results end-to-end.
    • Updated RBAC/search tests to normalize resource kind casing for consistent filter expectations.

Signed-off-by: Jorge Padilla <jpadilla@redhat.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1d4c5851-a1a0-439f-94fd-bcb772774850

📥 Commits

Reviewing files that changed from the base of the PR and between 7539760 and c38b016.

📒 Files selected for processing (1)
  • tests/api/rbac.test.js

📝 Walkthrough

Walkthrough

Adds runnable filter tests and parsing/matching helpers to tests/api/filter.test.js, normalizes expected values from command output, and updates RBAC tests to use capitalized ConfigMap in kind filters. No production code changes.

Changes

Filter Test Implementation

Layer / File(s) Summary
Test Helpers / Data Shape
tests/api/filter.test.js
Introduces parsing/matching utilities: operator-prefixed parsing, numeric/boolean handling, relative-date keywords, and assertItemsMatchFilter.
Expected Values Normalization
tests/api/filter.test.js
filtersRegistry entries compute expected values from command outputs and normalize with .toString().trim() for fields like podIP, hostIP, kubernetesVersion, memory, clusterIP, architecture, osImage, consoleURL.
Test Execution / Wiring
tests/api/filter.test.js
Replaces test.todo with an async test loop that builds queries (searchQueryBuilder), sends requests (sendRequest), extracts searchResult[0].items, and asserts via assertItemsMatchFilter.

RBAC Test Adjustments

Layer / File(s) Summary
Test Query Changes
tests/api/rbac.test.js
Updates tests to use kind: ['ConfigMap'] (capitalized) in search filters for consistency in two test locations. No production code modified.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I hopped through tests to tidy the plot,

Trimmed outputs, parsed ops — found what was sought.
Numbers, dates, and booleans align,
Todos now pass, the green checks shine.
A grateful rabbit leaves a tiny carrot-dot.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Re-enable the filters (auto-complete) tests' clearly and specifically describes the primary change: re-enabling filter-related tests that were previously disabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jorge/enable-filter-tests

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.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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

Copy link
Copy Markdown

@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: 2

🤖 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.

Inline comments:
In `@tests/api/filter.test.js`:
- Around line 59-83: parseFilterValue currently parses operators like '!=' and
'!' into operator but the numeric comparison block (using operator/operand,
isNumericOperand, expectedNum, actualNum) only implements =, >, <, >=, <=
causing '!=' and '!' to fall through to string matching; update the numeric
switch in the test (the switch over operator inside the items.forEach) to handle
'!=' and '!' by asserting not-equality (use Jest's
expect(actualNum).not.toBe(expectedNum)), and similarly extend the non-numeric
literal matching branch (the code handling property === fv around lines 88-90)
to treat '!' and '!=' as negation (assert the property does not equal the
operand); keep other behavior and preserve the existing error path (throw for
truly unsupported operators).
- Around line 49-55: The test currently only checks parseability for values when
RELATIVE_DATE_VALUES.has(fv); update the assertion to verify each parsed date
actually falls within the expected relative range for the selected fv
(hour/day/week/month/year). For each item in items (same loop using fv and
property), parse the value into a Date (Date.parse), compute now = Date.now()
and a lowerBound = now - durationMillis where durationMillis maps fv -> {hour:
1*60*60*1000, day: 24*60*60*1000, week: 7*24*60*60*1000, month: 30*24*60*60*1000
(or use month arithmetic), year: 365*24*60*60*1000 (or use full-year
arithmetic)}, then assert parsedTime <= now and parsedTime >= lowerBound; keep
the existing parse/assertions but replace the weak Date.parse check with these
range assertions so the test fails if the backend ignored the date filter.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a5b506b0-9f35-49aa-bdbc-fa2400c5a4c9

📥 Commits

Reviewing files that changed from the base of the PR and between 7ad9dbb and 4a0ab87.

📒 Files selected for processing (1)
  • tests/api/filter.test.js

Comment thread tests/api/filter.test.js
Comment thread tests/api/filter.test.js
1. Add proper relative date range validation - verify dates fall within
   the expected time window (hour/day/week/month/year ago from now)
2. Add support for != and ! operators in both numeric and non-numeric
   filter assertions

Addresses review comments in PR #461.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Jorge Padilla <jpadilla@redhat.com>
@jlpadilla
Copy link
Copy Markdown
Contributor Author

🤖 I'm an AI agent

I've addressed both CodeRabbit review comments:

1. Relative date range validation (lines 49-67)

  • ✅ Added proper time range validation for relative date filters
  • Now verifies that parsed dates fall within the expected window (hour/day/week/month/year ago from now)
  • Checks both upper bound (≤ now) and lower bound (≥ now - maxAge)

2. Operator support for != and ! (lines 82-84, 106-110)

  • ✅ Added support for != and ! operators in numeric comparisons using expect().not.toBe()
  • ✅ Added support for != and ! operators in non-numeric comparisons using negated itemValueMatches()
  • These operators were being parsed but not properly handled in assertions

Both issues have been fixed in commit 7539760.

@jlpadilla
Copy link
Copy Markdown
Contributor Author

/test deploy-and-run-e2e-api

jlpadilla added 2 commits May 6, 2026 12:31
Signed-off-by: Jorge Padilla <jpadilla@redhat.com>
Signed-off-by: Jorge Padilla <jpadilla@redhat.com>
@jlpadilla
Copy link
Copy Markdown
Contributor Author

Tests job is failing because of actual product regression bug. https://redhat.atlassian.net/browse/ACM-33881

@jlpadilla
Copy link
Copy Markdown
Contributor Author

/test deploy-and-run-e2e-api

@smcavey
Copy link
Copy Markdown
Contributor

smcavey commented May 12, 2026

Why remove available, port, and suspend queries?

@jlpadilla
Copy link
Copy Markdown
Contributor Author

Why remove available, port, and suspend queries?

@smcavey These were commented out. We have a known issue for available because the value can be either an integer or boolean. I don't recall the specific reason for port and suspend.

The goal of the test is to validate the API, not to check for every single property collected.

@smcavey
Copy link
Copy Markdown
Contributor

smcavey commented May 12, 2026

/lgtm

@openshift-ci openshift-ci Bot added the lgtm label May 12, 2026
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 12, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlpadilla, smcavey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@smcavey
Copy link
Copy Markdown
Contributor

smcavey commented May 12, 2026

/test deploy-and-run-e2e-api

@jlpadilla
Copy link
Copy Markdown
Contributor Author

/test deploy-and-run-e2e-api
/hold

Last 2 executions failed with different errors. Adding hold to run several times to confirm this isn't adding stability problems

@jlpadilla
Copy link
Copy Markdown
Contributor Author

/test all

@openshift-ci openshift-ci Bot removed the lgtm label May 14, 2026
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 14, 2026

New changes are detected. LGTM label has been removed.

@jlpadilla
Copy link
Copy Markdown
Contributor Author

/test deploy-and-run-e2e-api

Got another different random fail.

      77 |
      78 |         // Ensure we didn't exhaust retries (metric should have updated)
    > 79 |         expect(retries).toBeLessThan(maxRetries)
         |                         ^
      80 |
      81 |         // Verify metric content
      82 |         expect(resp.body.data.result[0].metric['__name__']).toEqual(metricsName)
      at Object.toBeLessThan (subscription-metrics.test.js:79:25)

@jlpadilla
Copy link
Copy Markdown
Contributor Author

/test deploy-and-run-e2e-api
Last error waiting for MCH deploy.

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 15, 2026

@jlpadilla: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/deploy-and-run-e2e-api 3d2439f link true /test deploy-and-run-e2e-api

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants