Skip to content

test(e2e): Add behaviour test for Errors in standard React E2E tests application #5909

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

Merged
merged 16 commits into from
Oct 11, 2022

Conversation

lforst
Copy link
Contributor

@lforst lforst commented Oct 7, 2022

Ref: #5855

Introduces a behaviour test in the standard-frontend-react E2E tests. This test triggers an error in the application using Playwright and verifies if an error event was sent to Sentry by polling for the event via the Sentry API.

We also jump through some hoops to inject the auth token required for polling Sentry into the test. It's so that we can configure the auth token either via an env var or via a file.

Next up we will verify the behaviour of transactions and sessions in additional tests: #5912

@lforst lforst added this to the E2E Tests milestone Oct 7, 2022
@lforst lforst mentioned this pull request Oct 7, 2022
2 tasks
@lforst lforst marked this pull request as ready for review October 7, 2022 12:23
if (!fs.existsSync(authTokenPath)) {
fs.writeFileSync(authTokenPath, JSON.stringify({ authToken: authTokenHint }, null, 2));
console.log(
'No auth token configured for E2E tests! Please set the E2E_TEST_AUTH_TOKEN environment variable or put your auth token in "auth-token.json"!',
Copy link
Member

Choose a reason for hiding this comment

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

m: Why not just use a .env file? We can detect process.env.NODE_ENV is CI and fail fast if we don't see a .env configured.

Copy link
Contributor Author

@lforst lforst Oct 7, 2022

Choose a reason for hiding this comment

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

Yeah you're right. I totally overcomplicated this. Changed it to just use env vars all the way! 018bccc

@@ -15,7 +15,8 @@ import Index from './pages/Index';
import User from './pages/User';

Sentry.init({
dsn: 'https://[email protected]/1337',
// DSN belongs to "e2e-javascript-standard-frontend-react" project in "sentry-sdks" org
dsn: 'https://[email protected]/4503941750587392',
Copy link
Member

Choose a reason for hiding this comment

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

l: Should we inject this instead of hardcoding?

m: Also, I'd rather just send everything to one project (not specify it to react), we can use sdk name/version to filter it in the Sentry UI.

Copy link
Contributor Author

@lforst lforst Oct 7, 2022

Choose a reason for hiding this comment

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

Thinking more about it I agree. Changed it so we're injecting the DSN via env var: 018bccc

}

const envVarsToInject = {
REACT_APP_E2E_TEST_DSN: process.env.E2E_TEST_DSN,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we do the renaming here? Shouldn't it just be called E2E_TEST_DSN everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since CRA apps are static we need to inject the DSN during build time (I guess that's more or less only possible with env vars) and CRA has restrictions on what env vars may look like: https://create-react-app.dev/docs/adding-custom-environment-variables/ (They only allow env vars prefixed with REACT_APP_)

@@ -0,0 +1 @@
.env
Copy link
Member

Choose a reason for hiding this comment

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

l: should we add an .env.example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 8561e30

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Nice, looks good to me!

@lforst lforst merged commit 8ef9995 into master Oct 11, 2022
@lforst lforst deleted the lforst-standard-react-application-behaviour-e2e-test branch October 11, 2022 08:18
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.

3 participants