Skip to content

Conversation

@lperson
Copy link
Collaborator

@lperson lperson commented Mar 31, 2024

Fixes # (issue)

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change, and any blockers that make your change a WIP

Checklist:

  • I have manually tested my changes on desktop and mobile
  • The test suite passes locally with my changes
  • If my change is a UI change, I have attached a screenshot to the description section of this pull request
  • My change is 300 lines of code or less, or has a documented reason in the description why it’s longer
  • I have made any necessary changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • My PR is labeled [WIP] if it is in progress

@lperson lperson changed the base branch from main to new-node-20-branch March 31, 2024 18:20
lperson added 3 commits March 31, 2024 14:36
* The excluded UI tests don't use server components

* We need `@jest-environment jsdom` for testing UI components where we
mount react components but it doesn't include setImmediate. The
Redis client needs setImmediate which is available when we use
`@jest-environment node` but then we can't mount react components.
One size does not fit all!
@lperson lperson marked this pull request as draft April 2, 2024 21:48
@lperson lperson marked this pull request as ready for review April 28, 2024 17:58
@lperson lperson requested a review from mau11 April 28, 2024 17:58
@engelhartrueben
Copy link
Collaborator

engelhartrueben commented Apr 29, 2024

Hey Larry,

Just want to first thank you for working on this! Absolute life saver!

I did get this all to pass locally (w/ NGPVAN) in two ways:
1. by setting the jest.setTimeout to 25,000ms in __test__/setup.js. This NGPVAN test looks to take around 20 seconds while running all the other tests alongside it, so some headroom was necessary.
2. by running __test__/extensions/contact-loaders/ngpvan/ngpvan.test.js by itself w/ the original timeout.

It seems this was extended in the past for one of the campaign tests from the default 5,000ms to 15,000ms.

Would it be viable to only allow the timeout extension for just the the NGPVAN test? Or could we comfortably extend it globally?

I understand it may not be best practice to just extend the timer without finding the root cause, so just exploring options!

Copy link
Collaborator

@mau11 mau11 left a comment

Choose a reason for hiding this comment

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

lgtm!

@engelhartrueben
Copy link
Collaborator

engelhartrueben commented May 5, 2024

If you have a second @mau11 , my last commit needs to be removed. No point in extending the jest timeout since that was never the problem. I can get to it later tonight if not!

@lperson
Copy link
Collaborator Author

lperson commented May 5, 2024

@engelhartrueben I changed the timeout back to 15000

@lperson lperson force-pushed the redis-updates branch 4 times, most recently from c8b7162 to 707c3a3 Compare May 6, 2024 13:09
@lperson lperson force-pushed the redis-updates branch 4 times, most recently from 33bfcab to 7f6c80c Compare May 11, 2024 18:13
@lperson lperson merged commit b7932fc into ProgressiveCoders:new-node-20-branch May 11, 2024
@engelhartrueben
Copy link
Collaborator

🎉🎉🎉

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