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

feat(nextjs): Add ability for integration tests to use linked @sentry/xxxx packages #4019

Merged

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Sep 30, 2021

In the nextjs integration tests, we use file dependencies for all of the packages in the sentry-javascript repo, so that the tests test the local (rather than published) version of the SDK. We don't do the same for @sentry/cli or @sentry/webpack-plugin, though, because they're in a separate repo and we can't predict where the local copy of that repo lives. As a result, we currently can't (in the nextjs integration tests, at least) test any local changes in either package.

This solves that problem by optionally linking to the local copies of those repos as part of the integration test runner script. In order to use this optional linking:

  • To link @sentry/cli, set LINKED_CLI_REPO=<abs path of local sentry-cli repo>.
  • To link @sentry/webpack-plugin, set the CLI variable above (since @sentry/cli is a dependency of @sentry/webpack-plugin, we need to link it in the plugin repo also) as well as LINKED_PLUGIN_REPO=<abs path of local sentry-webpack-plugin repo>

@github-actions
Copy link
Contributor

github-actions bot commented Sep 30, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 22.32 KB (-0.01% 🔽)
@sentry/browser - Webpack 23.3 KB (0%)
@sentry/react - Webpack 23.33 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 29.77 KB (-0.01% 🔽)

@kamilogorek
Copy link
Contributor

Overall looks good, but I'm not sure if we need 2 checks everywhere.
One decides whether to link, second decides where to link. But the second argument is redundant without the first, and the first is redundant without the second. So maybe we can just use CLI_PATH and PLUGIN_PATH for both things? If it's present, link it, if it's not, then ignore it.
We can also optionally add [ ! -d $CLI_PATH ] && echo "CLI path $CLI_PATH does not exists" or similar check.

packages/nextjs/test/integration_test_utils.sh Outdated Show resolved Hide resolved
packages/nextjs/test/integration_test_utils.sh Outdated Show resolved Hide resolved
@lobsterkatie
Copy link
Member Author

Overall looks good, but I'm not sure if we need 2 checks everywhere. One decides whether to link, second decides where to link. But the second argument is redundant without the first, and the first is redundant without the second. So maybe we can just use CLI_PATH and PLUGIN_PATH for both things? If it's present, link it, if it's not, then ignore it. We can also optionally add [ ! -d $CLI_PATH ] && echo "CLI path $CLI_PATH does not exists" or similar check.

Yeah, that's actually what I had at first (one variable for each repo), but then I couldn't find a good variable name for the combo, so I made it two. You're right that it's probably less of a pain to use if it's just one. I can switch it back.

@lobsterkatie lobsterkatie changed the title feat(nextjs): Add ability for integration tests to use linked @sentry/x packages feat(nextjs): Add ability for integration tests to use linked @sentry/xxxx packages Oct 1, 2021
@lobsterkatie lobsterkatie merged commit 389ae60 into master Oct 1, 2021
@lobsterkatie lobsterkatie deleted the kmclb-nextjs-allow-linked-packages-in-integration-tests branch October 1, 2021 20:11
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