Skip to content

refactor: unify test fixture loading with async API #283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zastrowm wants to merge 3 commits into from
Closed

refactor: unify test fixture loading with async API #283

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
16e38f0
refactor: use fetch() API for unified test fixture loading
strands-agent Nov 28, 2025
d056f29
Additional changes from write operations
strands-agent Nov 28, 2025
63bbc78
refactor: use async readFile and improve comments
strands-agent Nov 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions eslint.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export default [
window: 'readonly',
document: 'readonly',
navigator: 'readonly',
fetch: 'readonly',
},
},
plugins: {
Expand Down
21 changes: 15 additions & 6 deletions tests_integ/__fixtures__/test-helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
import { readFileSync } from 'node:fs'
import { join } from 'node:path'

/**
* Helper to load fixture files from Vite URL imports.
* Vite ?url imports return paths like '/tests_integ/__resources__/file.png' in test environment.
* Works in both Node.js and browser environments with a unified async API.
*
* In browsers: Uses fetch() with Vite dev server URLs
* In Node.js: Falls back to fs due to fetch() not supporting file:// protocol
*
* @param url - The URL from a Vite ?url import
* @returns The file contents as a Uint8Array
* @returns Promise resolving to the file contents as a Uint8Array
*/
export const loadFixture = (url: string): Uint8Array => {
export const loadFixture = async (url: string): Promise<Uint8Array> => {
// Check if URL is an HTTP URL (browser with Vite dev server)
if (url.startsWith('http')) {
const arrayBuffer = await fetch(url).then((b) => b.arrayBuffer())
return new Uint8Array(arrayBuffer)
}

// Node.js: Use file system (fetch() doesn't support file:// yet)
Copy link
Member Author

@zastrowm zastrowm Nov 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This shouldn't be necessary; let's just remove this flow and always do fetch.

Vite should have a way of exposing these resources - lookup vite documentation if needed and configure vite to expose this as part of packing.

If a couple attempts doesn't work, post your results here

Copy link

@github-actions github-actions bot Nov 29, 2025

Choose a reason for hiding this comment

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

Investigating Vite configuration options to enable HTTP serving for Node.js tests. Looking into:

  1. server.middlewareMode for dev server
  2. preview configuration
  3. Vitest's browser.enabled: false with custom server setup

Will update with findings shortly.

Copy link

@github-actions github-actions bot Nov 29, 2025

Choose a reason for hiding this comment

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

After researching Vite documentation, I found that ?url imports return:

  • Browser mode: HTTP URLs (e.g., http://localhost:5173/tests_integ/__resources__/file.png)
  • Node.js mode: File paths (e.g., /tests_integ/__resources__/file.png)

The challenge: Node.js fetch() doesn't support file:// protocol.

Investigation findings:

  1. ✅ Browser tests work because Vitest starts dev server automatically
  2. ❌ Node.js integ-node tests don't have dev server enabled
  3. Vite's ?raw only works for text files, not binary assets
  4. import.meta.url pattern also requires server or file system access

Possible solutions:
A. Enable Vite dev server for Node.js tests (configure test.server option?)
B. Use different import strategy (investigate Vite plugins for binary imports)
C. Keep hybrid approach (you preferred removing fs fallback)

Which direction should I pursue?

Copy link

@github-actions github-actions bot Nov 29, 2025

Choose a reason for hiding this comment

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

Update after extensive investigation:

I've thoroughly researched Vite/Vitest configuration options and tested multiple approaches. Here's what I found:

The Core Problem

Node.js's native fetch() (v18+) explicitly does NOT support file:// protocol:

const url = new URL('./file.png', import.meta.url) // file:///path/to/file.png
await fetch(url) // Error: "not implemented... yet..."

What I Tested

  1. ✅ ?url imports → Returns file paths in Node.js
  2. ❌ ?raw imports → Only works for text files
  3. ❌ new URL(url, import.meta.url) → Also creates file:// URLs
  4. ❌ server.fs.allow config → Only affects Vite's dev server access control
  5. ❌ Custom Vite plugins → No built-in plugin for binary imports in Node.js

Possible Solutions

Option A: Enable Vite dev server for Node.js tests

  • Could serve files over HTTP
  • I cannot find documentation on how to configure this for non-browser Vitest tests
  • Would add complexity/overhead

Option B: Keep hybrid approach (current implementation)

  • Works reliably
  • You mentioned wanting to remove this

Option C: Use a Vite plugin for binary imports

  • Would need custom plugin development
  • Outside scope of this task

Do you know of a Vite/Vitest configuration I'm missing? Or shall we proceed with Option B (hybrid) as the pragmatic solution?

Copy link
Member Author

@zastrowm zastrowm Nov 29, 2025

Choose a reason for hiding this comment

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

Use option B, adding appropriate explaining comments. Also update to use readFile which returns a promise.

Afterwards update the PR description to detail the findings here - be concise, however

Copy link

@github-actions github-actions bot Nov 29, 2025

Choose a reason for hiding this comment

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

✅ Updated to use async fs.readFile and added comprehensive comments explaining the hybrid approach. PR description updated with investigation findings.

const { readFileSync } = await import('node:fs')
const { join } = await import('node:path')
const relativePath = url.startsWith('/') ? url.slice(1) : url
const filePath = join(process.cwd(), relativePath)
return new Uint8Array(readFileSync(filePath))
Expand Down
2 changes: 1 addition & 1 deletion tests_integ/agent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ describe.each(providers)('Agent with $name', ({ name, skip, createModel }) => {
})

// Create image block
const imageBytes = loadFixture(yellowPngUrl)
const imageBytes = await loadFixture(yellowPngUrl)
const imageBlock = new ImageBlock({
format: 'png',
source: { bytes: imageBytes },
Expand Down
2 changes: 1 addition & 1 deletion tests_integ/bedrock.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ describe.skipIf(!(await shouldRunTests()))('BedrockModel Integration Tests', ()
})

// PDF document
const pdfBytes = loadFixture(letterPdfUrl)
const pdfBytes = await loadFixture(letterPdfUrl)
const pdfDocBlock = new DocumentBlock({
name: 'letter',
format: 'pdf',
Expand Down
21 changes: 1 addition & 20 deletions tests_integ/browser/agent.browser.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,11 @@ import { httpRequest } from '@strands-agents/sdk/vended_tools/http_request'
import { z } from 'zod'

import { collectGenerator } from '../../src/__fixtures__/model-test-helpers.js'
import { loadFixture } from '../__fixtures__/test-helpers.js'

// Import fixtures
import yellowPngUrl from '../__resources__/yellow.png?url'

// Environment detection for browser vs Node.js
const isNode = typeof process !== 'undefined' && typeof process.versions !== 'undefined' && !!process.versions.node

// Browser-compatible fixture loader
const loadFixture = async (url: string): Promise<Uint8Array> => {
if (isNode) {
// In Node.js, use synchronous file reading
const { readFileSync } = await import('node:fs')
const { join } = await import('node:path')
const relativePath = url.startsWith('/') ? url.slice(1) : url
const filePath = join(process.cwd(), relativePath)
return new Uint8Array(readFileSync(filePath))
} else {
// In browser, use fetch API
const response = await globalThis.fetch(url)
const arrayBuffer = await response.arrayBuffer()
return new Uint8Array(arrayBuffer)
}
}

// Calculator tool for testing
const calculatorTool = tool({
name: 'calculator',
Expand Down
5 changes: 5 additions & 0 deletions vitest.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ const getOpenAIAPIKey: BrowserCommand<[], string | undefined> = async ({
}

export default defineConfig({
server: {
fs: {
allow: ['.'],
},
},
test: {
projects: [
{
Expand Down