-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
test(remix): Run tsc
before Remix E2E tests
#16345
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
base: develop
Are you sure you want to change the base?
Conversation
@@ -20,7 +20,7 @@ test('Sends parameterized transaction name to Sentry', async ({ page }) => { | |||
|
|||
test('Sends form data with action span', async ({ page }) => { | |||
const formdataActionTransaction = waitForTransaction('create-remix-app-express', transactionEvent => { | |||
return transactionEvent?.spans?.some(span => span.data && span.data['code.function'] === 'action'); | |||
return transactionEvent?.spans?.some(span => span.data && span.data['code.function'] === 'action') || false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: What does adding || false
do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waitForTransaction
expects a function returning a boolean
, after adding optional chaining, it became boolean | undefined
. So, making it false
in such cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... of course 😅 thanks
...kages/e2e-tests/test-applications/create-remix-app-express/tests/server-transactions.test.ts
Show resolved
Hide resolved
@andreiborza - this branch should fail on CI actually. And it does fail on my local. Looks like CI is letting |
ab4b1ac
to
8fb25f1
Compare
Update: I was not able to make this fail on CI (with |
Following up: #16336