Skip to content
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.

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

fix(node): LocalVariables integration should use setupOnce #10307

Merged
merged 5 commits into from
Jan 24, 2024

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Jan 24, 2024

Closes #10278

This integration incorrectly used setup which then caused issues when the core code was changed. It should have been using setupOnce.

Unfortunately, setupOnce causes issues with the unit tests since it then relies on getClient to get the client instance for the client options. I've tried using jest to mock getClient but this doesn't work which is likely due to installedIntegrations.

Because we already have extensive integration tests for LocalVariables I have removed the unit tests because they are becoming a maintenance burden and actually test less than the integration tests.

After v8, I plan to move Local variables lookup to the worker thread at which point we can revisit how these might be better tested in unit tests.

@timfish timfish marked this pull request as ready for review January 24, 2024 12:54
@AbhiPrasad AbhiPrasad merged commit d471dcf into getsentry:develop Jan 24, 2024
53 checks passed
@timfish timfish deleted the fix/localvariables-setup-once branch January 24, 2024 14:16
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.

Local variables issue: The inspector session is already connected
2 participants