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

Exclude test environments from Sentry reporting #140

Merged
merged 4 commits into from
May 20, 2024

Conversation

derekblank
Copy link
Contributor

Proposed Changes

Reduce Sentry noise by excluding test environments:

  • Adds process.env.NODE_ENV !== 'test' to Sentry init config
  • Prepends package.json test commands with NODE_ENV=test

Testing Instructions

Run testing commands like npm run e2e or npm run test to observe that Sentry events are not reported.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@derekblank derekblank added [Type] Enhancement Improvement upon an existing feature Tooling labels May 17, 2024
@derekblank derekblank requested review from dcalhoun and a team May 17, 2024 06:19
Copy link
Contributor

@wojtekn wojtekn left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the change.

I see we use process.env.E2E in some other places. Would it make sense to unify this to use one or another everywhere?

package.json Outdated
Comment on lines 27 to 28
"test": "NODE_ENV=test jest",
"test:watch": "NODE_ENV=test jest --watch",
Copy link
Member

@dcalhoun dcalhoun May 17, 2024

Choose a reason for hiding this comment

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

From reviewing documentation and testing locally, defining this explicitly is unnecessary as Jest sets NODE_ENV=test if it is not set to something else.

Jest will set process.env.NODE_ENV to 'test' if it's not set to something else.

That said, I'm fine keeping this here to be explicit, if preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see we use process.env.E2E in some other places. Would it make sense to unify this to use one or another everywhere?
From reviewing documentation and testing locally, defining this explicitly is unnecessary as Jest sets NODE_ENV=test if it is not set to something else.

Good points, both. I think it makes sense to leverage the existing process.env.E2E event for E2E tests -- I hadn't yet noted that we were already using it. I'll update both areas and check that the env is being set prior to running E2E builds, as noted in #140 (comment).

package.json Outdated Show resolved Hide resolved
Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

Nice work! From my testing, the latest changes appear to accomplish the desired outcome. Thank you for reducing the error logging noise.

Testing diff
diff --git a/e2e/e2e-helpers.ts b/e2e/e2e-helpers.ts
index feb764d..948e348 100644
--- a/e2e/e2e-helpers.ts
+++ b/e2e/e2e-helpers.ts
@@ -4,6 +4,7 @@ import { _electron as electron } from 'playwright';
 export async function launchApp( testEnv: NodeJS.ProcessEnv = {} ) {
 	// find the latest build in the out directory
 	const latestBuild = findLatestBuild();
+	console.log( '>>>', { E2E: process.env.E2E, NODE_ENV: process.env.NODE_ENV } );
 
 	// parse the packaged Electron app and find paths and other info
 	const appInfo = parseElectronApp( latestBuild );
diff --git a/src/index.ts b/src/index.ts
index e9ff8b4..d64da96 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -6,6 +6,7 @@ import {
 	type IpcMainInvokeEvent,
 	globalShortcut,
 	Menu,
+	dialog,
 } from 'electron';
 import path from 'path';
 import * as Sentry from '@sentry/electron/main';
@@ -26,6 +27,12 @@ import setupWPServerFiles from './setup-wp-server-files';
 import { setupUpdates } from './updates';
 import { stopAllServersOnQuit } from './site-server'; // eslint-disable-line import/order
 
+app.whenReady().then( () => {
+	dialog.showMessageBox( {
+		message: `E2E ${ process.env.E2E } / ENV ${ process.env.NODE_ENV }`,
+	} );
+} );
+
 Sentry.init( {
 	dsn: 'https://97693275b2716fb95048c6d12f4318cf@o248881.ingest.sentry.io/4506612776501248',
 	debug: true,
Testing outcome
❯ npm run e2e -- app.test.ts

> studio@1.0.2 e2e
> npx playwright install && npx playwright test app.test.ts


Running 1 test using 1 worker

  ✓  1 app.test.ts:18:6 › Electron app › should ensure app title is correct. (228ms)
>>> { E2E: undefined, NODE_ENV: undefined }

  1 passed (8.3s)
Screenshot 2024-05-20 at 14 44 32

@dcalhoun dcalhoun merged commit b9642d8 into trunk May 20, 2024
11 checks passed
@dcalhoun dcalhoun deleted the feat/exclude-sentry-test-env branch May 20, 2024 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tooling [Type] Enhancement Improvement upon an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants