Skip to content

feat: filter test ci#4599

Closed
devinat1 wants to merge 9 commits intomainfrom
filter-test-ci
Closed

feat: filter test ci#4599
devinat1 wants to merge 9 commits intomainfrom
filter-test-ci

Conversation

@devinat1
Copy link
Contributor

This pull request introduces automated testing for the filters package.

Continuous Integration Setup:

  • Added a new GitHub Actions workflow (.github/workflows/filters-test.yml) to run type checking, linting, and tests for the filters package, including matrix testing across Node.js versions and test suite partitioning for better coverage and reporting.

@vercel
Copy link

vercel bot commented Aug 29, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
helicone Canceled Canceled Aug 29, 2025 0:42am
helicone-bifrost Canceled Canceled Aug 29, 2025 0:42am
helicone-eu Canceled Canceled Aug 29, 2025 0:42am

@claude
Copy link
Contributor

claude bot commented Aug 29, 2025

Claude finished @devinat1's task —— View job


PR Review: Filter Test CI

Todo List:

  • Review GitHub Actions workflow configuration
  • Analyze test file structure and coverage
  • Review individual test files for quality
  • Check for bugs, security issues, and performance concerns
  • Provide final feedback with confidence score

Summary

This PR adds comprehensive test coverage for the filters package (~2100 lines of tests for ~2400 lines of source code) but has critical CI configuration issues that will prevent it from working.

Major Issues

🔴 Critical CI Configuration Problems (Score Impact: -4)
  1. Missing dependencies in package.json: Tests import from @helicone-package/cost and other modules not listed in dependencies
  2. Workflow runs from wrong directory: Commands run in ./packages/filters but Jest config is at ./packages/jest.config.ts
  3. Redundant test execution: Both jobs run identical tests, wasting CI resources
  4. Branch in trigger conditions: Including filter-test-ci in push/PR triggers is unusual for production workflows
  5. No proper workspace setup: Workflow doesn't leverage yarn workspace structure

Fix needed: Update workflow to run from packages root and use proper Jest config path

🟡 Test Quality Assessment (Good overall)

Positives:

  • Excellent test coverage (87% of source code lines)
  • Comprehensive edge case testing (XSS strings, Unicode, large numbers, null values)
  • Well-structured test organization by functionality
  • Good use of describe blocks and clear test naming
  • Tests critical FilterAST operations and helper functions

Minor concerns:

  • Some tests may have API mismatches (need verification against actual implementation)
  • Heavy reliance on exact object structure matching (brittle to refactoring)
  • Tests import from packages not in dependencies
Security & Performance

Security: ✅ No security concerns. Tests appropriately handle XSS strings and special characters.

Performance: ⚠️ Tests create many complex nested objects but this is acceptable for testing. CI will run unnecessarily long due to redundant job execution.

Recommendations

  1. Fix CI workflow immediately - update working directory and dependencies
  2. Remove redundant test-individual job or restructure for actual parallelization
  3. Add proper Jest/TypeScript dependencies to filters package.json
  4. Consider using workspace-level test script instead of package-specific Jest config

Confidence Score: 3/10

Reason for low score: Critical CI configuration issues will cause build failures. While the test quality is good, the workflow cannot run successfully as configured. The PR needs significant fixes before merge.

The comprehensive test suite shows excellent attention to detail and coverage, but the infrastructure setup requires immediate attention to make this PR functional.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR introduces comprehensive automated testing infrastructure for the filters package, which is a critical component of Helicone's data filtering system used throughout the application for dashboard analytics, request logs, user metrics, and session tracking. The changes add five new test files covering different aspects of the filters package: basic utility functions (filters.test.ts), frontend filter definitions (frontendFilterDefs.test.ts), helper functions (helpers.test.ts), filter expressions (filterExpressions.test.ts), and edge cases (edgeCases.test.ts).

The testing suite provides extensive coverage of the filter system's functionality, including type definitions, filter generators, predefined configurations, UI transformations, FilterAST operations, error handling, and performance scenarios. The tests validate critical functionality like converting UI filter representations to database queries, handling custom properties and feedback filters, and managing complex nested filter structures.

A new GitHub Actions workflow (.github/workflows/filters-test.yml) has been added to run type checking, linting, and tests with matrix testing across Node.js versions and partitioned test suites for better coverage reporting. The workflow integrates with Codecov for test coverage tracking. Additionally, coverage/ has been added to .gitignore to exclude generated test coverage reports from version control.

Confidence score: 2/5

  • This PR has significant implementation risks that could cause CI pipeline failures and testing issues
  • Score lowered due to potential undefined npm scripts in the workflow, possible API mismatches in tests, and workflow configuration concerns
  • Pay close attention to .github/workflows/filters-test.yml and packages/filters/__tests__/filterExpressions.test.ts

Context used:

Context - When naming jobs in GitHub Actions, prefer descriptive names that clearly indicate the job's purpose, such as 'Build Docker image' instead of generic terms like 'Precheck'. (link)

7 files reviewed, 2 comments

Edit Code Review Bot Settings | Greptile


on:
push:
branches: [ main, develop, filter-test-ci ]
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Including filter-test-ci branch in push triggers is unusual for production workflows - typically only main/develop branches are included

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed


test-suites:
runs-on: ubuntu-latest
needs: test
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The needs: test dependency may be unnecessary if test-suites job is meant to run independently - could slow down CI execution

paths:
- 'packages/filters/**'
pull_request:
branches: [ main, develop, filter-test-ci ]
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@chitalian
Copy link
Contributor

  • Workflow runs from wrong directory: Commands run in ./packages/filters but Jest config is at ./packages/jest.config.ts
  • Redundant test execution: Both jobs run identical tests, wasting CI resources
  • Branch in trigger conditions: Including filter-test-ci in push/PR triggers is unusual for production workflows

@devinat1 these seem like real things @claude found

@claude
Copy link
Contributor

claude bot commented Aug 29, 2025

Claude finished @chitalian's task —— View job


Analyzing CI Configuration Issues

Todo List:

  • Read and analyze the workflow configuration
  • Examine package structure and dependencies
  • Provide specific fixes for identified issues
  • Summarize recommendations

You're absolutely right @chitalian - those are legitimate configuration issues that will prevent the CI from working properly.

Confirmed Critical Issues

  1. Wrong working directory: Workflow runs from ./packages/filters but Jest config is at ./packages/jest.config.ts (parent level)
  2. Redundant execution: Both test and test-individual jobs run identical tests - wasting CI resources
  3. Inappropriate branch trigger: filter-test-ci shouldn't be in production workflow triggers
  4. Missing dependencies: The filters package.json has only 4 lines - no Jest, TypeScript, or test dependencies
  5. Incorrect npm install: Runs npm install in filters directory when this is a yarn workspace

Specific Fixes Needed

Workflow Configuration (.github/workflows/filters-test.yml):

# Fix working directory and use workspace structure
- name: Install dependencies
  run: yarn install

- name: Run tests  
  run: yarn jest packages/filters/__tests__/ --config=packages/jest.config.ts

# Remove redundant test-individual job entirely
# Remove filter-test-ci from branch triggers

Package Structure:

  • Filters package should inherit dependencies from workspace root
  • Tests should run from packages root using shared Jest config
  • Use yarn commands instead of npm in a yarn workspace

Impact

Without these fixes, the CI will fail because:

  • Jest config won't be found from wrong directory
  • Dependencies won't install properly
  • Tests will run twice unnecessarily
  • Branch triggers include temporary branch name

The test quality is excellent (~2100 lines covering critical filter functionality), but the infrastructure setup needs these fixes to be functional.


@devinat1
Copy link
Contributor Author

devinat1 commented Sep 7, 2025

Will create a larger CI PR covering all tests we currently have to replace this one.

@devinat1 devinat1 closed this Sep 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants