Skip to content

test: Remove old auto instrumentation e2e tests#1722

Open
Stephen Belanger (Qard) wants to merge 2 commits intomainfrom
fix/static-e2e-fixtures
Open

test: Remove old auto instrumentation e2e tests#1722
Stephen Belanger (Qard) wants to merge 2 commits intomainfrom
fix/static-e2e-fixtures

Conversation

@Qard
Copy link
Copy Markdown
Contributor

Summary

  • The end-to-end auto-instrumentation tests were generating fixture scripts at runtime via fs.writeFileSync, then cleaning up with fs.unlinkSync
  • When tests failed, the cleanup never ran, leaving stale untracked .mjs files in the fixtures directory
  • Moves each fixture to a static file committed alongside the test, removes the dynamic generate/cleanup logic from the test

Test plan

  • cd js && pnpm test — end-to-end tests reference static fixture files directly
  • No more untracked files after a test failure

🤖 Generated with Claude Code

The end-to-end auto-instrumentation tests were using fs.writeFileSync
to generate fixture scripts at test time, then fs.unlinkSync to clean up.
When tests failed the cleanup never ran, leaving stale untracked files.

Move each fixture to a static .mjs file in the fixtures/ directory and
remove the dynamic write/unlink calls from the test.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@lforst Luca Forstner (lforst) left a comment

Choose a reason for hiding this comment

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

Do you think instead of having these tests at all we could move them to the "new" E2E tests?

In general these tests seem to operate on a weird assertion level - like call api from user perspective but assert on an internal. I don't particularly like that.

These tests called the AI SDK from a user perspective but asserted on
internal SPAN_DATA output via subprocess stdout parsing — an awkward
level of assertion. Real coverage for this already exists in the e2e/
scenarios (anthropic-instrumentation, openai-instrumentation, etc.).

Remove the end-to-end.test.ts file and its fixtures entirely.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Qard
Copy link
Copy Markdown
Contributor Author

Stephen Belanger (Qard) commented Apr 2, 2026

I've removed those. They were also validating a channel name mismatch though, which are implementation details, but also represents a real bug which probably should be covered. Should we check that internal value in the true e2e tests, or do you feel the e2e test passing is sufficient evidence of that being correct?

I generally prefer more unit tests as they can catch a lot of cases where a thing works by accident and you don't realize until much later that it's been subtly broken and just not in a way that e2e tests reflected.

@lforst
Copy link
Copy Markdown
Member

Should we check that internal value in the true e2e tests, or do you feel the e2e test passing is sufficient evidence of that being correct?

In this particular case, I think the e2e tests are enough because with the current architecture, if the channels don't match the e2e tests will scream at you very loudly.

I generally prefer more unit tests as they can catch a lot of cases where a thing works by accident and you don't realize until much later that it's been subtly broken and just not in a way that e2e tests reflected.

I personally like to have both. For things that are side-effect free and have a limited scope, I like unit tests. For everything that may be influenced by external factors and/or is more complicated and/or has side-effects, I usually prefer integration/e2e tests. I always gravitate towards the latter more because the coverage you get is much broader usually and also more reflects how users are seeing our product, which is arguably the most important thing.

@lforst Luca Forstner (lforst) changed the title fix(tests): convert dynamically-generated e2e fixtures to static files test: Remove old auto instrumentation e2e tests Apr 2, 2026
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