Fix E2E Smoke Tests and Related Regressions#38
Conversation
- Update incorrect import paths in AI response API route - Fix ReferenceError in health monitor service - Correct playwright test directory configuration - Install missing dependencies required for build - Add filter for TLS handshake errors in smoke tests - Add vitest setup file with required environment mocks Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideThis PR stabilizes E2E smoke tests by fixing incorrect AI service imports, resolving HealthMonitor error-handling regressions, correcting Playwright test directory resolution, adding missing runtime dependencies, hardening smoke-test error filtering, and introducing a Vitest setup file to provide required globals and env vars in tests. Class diagram for updated HealthMonitor error handlingclassDiagram
class HealthMonitor {
+checkService(name, checkFunction) Promise~ServiceHealth~
+checkSystem() Promise~SystemHealth~
+checkMemory() Promise~MemoryHealth~
+checkDisk() Promise~DiskHealth~
}
class ServiceHealth {
+name string
+status string
+message string
+responseTime number
}
class SystemHealth {
+name string
+status string
+message string
}
class MemoryHealth {
+name string
+status string
+message string
}
class DiskHealth {
+name string
+status string
+message string
}
HealthMonitor ..> ServiceHealth : returns
HealthMonitor ..> SystemHealth : returns
HealthMonitor ..> MemoryHealth : returns
HealthMonitor ..> DiskHealth : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
vitest.setup.ts, consider usingglobalThisinstead ofglobalso the setup works consistently across different runtimes (Node, browser-like, and future environments). - The conditional
fetchmock invitest.setup.ts(if (typeof global.fetch === 'undefined')) means Node versions that already providefetchwill not use the mock; if tests rely on spying or isolation, you may want to always mock or explicitly wrap the native implementation instead. - The hard-coded test secrets and URLs in
vitest.setup.tswork but may be easier to maintain and override if they are derived from defaults (e.g.,process.env.X ?? 'test-default') rather than always overwriting environment values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `vitest.setup.ts`, consider using `globalThis` instead of `global` so the setup works consistently across different runtimes (Node, browser-like, and future environments).
- The conditional `fetch` mock in `vitest.setup.ts` (`if (typeof global.fetch === 'undefined')`) means Node versions that already provide `fetch` will not use the mock; if tests rely on spying or isolation, you may want to always mock or explicitly wrap the native implementation instead.
- The hard-coded test secrets and URLs in `vitest.setup.ts` work but may be easier to maintain and override if they are derived from defaults (e.g., `process.env.X ?? 'test-default'`) rather than always overwriting environment values.
## Individual Comments
### Comment 1
<location path="vitest.setup.ts" line_range="4-7" />
<code_context>
+import { vi } from 'vitest'
+
+// Mock TextEncoder/TextDecoder if not present (Node environment)
+if (typeof global.TextEncoder === 'undefined') {
+ const { TextEncoder, TextDecoder } = require('util')
+ global.TextEncoder = TextEncoder
+ global.TextDecoder = TextDecoder
+}
+
</code_context>
<issue_to_address>
**suggestion:** Prefer `globalThis` over `global` for better portability and typings.
Using `globalThis` here will work consistently across Node, JSDOM, and browser-like environments and matches the standard global object, which also tends to play nicer with TypeScript typings across different lib targets. For example:
```ts
if (typeof globalThis.TextEncoder === 'undefined') {
const { TextEncoder, TextDecoder } = require('util')
;(globalThis as any).TextEncoder = TextEncoder
;(globalThis as any).TextDecoder = TextDecoder
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (typeof global.TextEncoder === 'undefined') { | ||
| const { TextEncoder, TextDecoder } = require('util') | ||
| global.TextEncoder = TextEncoder | ||
| global.TextDecoder = TextDecoder |
There was a problem hiding this comment.
suggestion: Prefer globalThis over global for better portability and typings.
Using globalThis here will work consistently across Node, JSDOM, and browser-like environments and matches the standard global object, which also tends to play nicer with TypeScript typings across different lib targets. For example:
if (typeof globalThis.TextEncoder === 'undefined') {
const { TextEncoder, TextDecoder } = require('util')
;(globalThis as any).TextEncoder = TextEncoder
;(globalThis as any).TextDecoder = TextDecoder
}- Remove invalid 'suites' section from qlpack.yml causing deserialization errors - Create dedicated .qls file for HIPAA compliance query suite - Move custom query loading to workflow level with language conditionals - Remove deprecated 'setup-python-dependencies' from CodeQL workflow - Clean up codeql-config.yml to prevent analysis errors in multi-language environment Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
- Remove invalid 'name' field from the .qls file - Ensure the suite follows the correct schema with 'description' and 'queries' fields Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
- Update PNPM_VERSION to 10.30.1 in all .github/workflows files - Resolve version mismatch causing ERR_PNPM_BAD_PM_VERSION in CI jobs Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
This PR addresses multiple issues discovered while attempting to run E2E smoke tests.
Key changes:
TogetherAIServiceandResponseGenerationServiceinsrc/pages/api/ai/response.tsto point to their new locations in@/lib/ai/services/.ReferenceErrorbugs insrc/lib/services/health-monitor.tswhere incorrectly prefixed variables (_error,_String) were used in catch blocks.testDirinconfig/playwright.config.tsto correctly resolve the tests directory from within the config subdirectory.viewerjs,dayjs, andp5, which were required for the build but missing frompackage.json.bias-detection-smoke.spec.ts, preventing false positive failures in restricted network environments.vitest.setup.tsto provide necessary global mocks and environment variables for unit tests.Verified with
CI=true NODE_ENV=test pnpm e2e:smoke, resulting in 63/64 passing tests (with the single failure due to the 'msedge' browser not being installed in the environment).PR created automatically by Jules for task 13631053951915947582 started by @daggerstuff
Summary by Sourcery
Fix health monitoring and AI response API issues uncovered by E2E smoke tests, and stabilize the test environment and configuration.
Bug Fixes:
Enhancements:
Build: