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(jest-environment): Support configuration of happy-dom with testEnvironmentOptions #1287

Merged

Conversation

Codex-
Copy link
Contributor

@Codex- Codex- commented Mar 5, 2024

This PR allows users of @happy-dom/jest-environment to pass through non-deprecated configuration options from IBrowserSettings to happy-dom via testEnvironmentOptions in the jest config.

This retains the existing behaviour of setting the default localhost URL on the instance.

Looking into testing, since the existing tests for this package are configured more like integration tests what would your preferred testing situation be? I add unit tests and set that up for this package to test those? Or add a new integration suite?

@Codex- Codex- requested a review from capricorn86 as a code owner March 5, 2024 06:07
@Codex- Codex- force-pushed the jest-environment_support_full_config branch from 5d37e60 to 44d2fd9 Compare March 5, 2024 06:08
@Codex- Codex- force-pushed the jest-environment_support_full_config branch from dc53c5e to ff9b0ed Compare March 11, 2024 20:43
Copy link
Owner

@capricorn86 capricorn86 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @Codex-! 🌟

I changed the solution to something simpler that I don't think will require any unit tests for now.

Instead of setting each option on the "window.happyDOM" property, I changed so that the object is sent into the Window constructor directly instead.

I hope you are fine with the solution 🙂

@capricorn86 capricorn86 merged commit 16396f9 into capricorn86:master Mar 11, 2024
3 checks passed
@Codex-
Copy link
Contributor Author

Codex- commented Mar 11, 2024

Instead of setting each option on the "window.happyDOM" property, I changed so that the object is sent into the Window constructor directly instead.

I did consider this but went the route of offering feedback to the consumer should the configuration be invalid. If you typo a key or use an invalid option, does this get bubbled up by happydom still?

I hope you are fine with the solution 🙂

It's simpler but at the cost of providing feedback to the consumer, and forces errorCapture to disabled.

The usage of errorCapture is actually my main motivator for adding this, as I have been trying to test error boundaries with react-router-dom so setting this to disabled works for my case.

I feel as though it should respect the setting specified in the config instead of being force disabled https://github.com/capricorn86/happy-dom/pull/1287/files#diff-ca9251b53f292fc136910984f98fa20367077cf2f260010ca5cf707d047a6a69R57

Flipping it to:

			settings: {
				errorCapture: BrowserErrorCaptureEnum.disabled,
				...(<IOptionalBrowserSettings>projectConfig.testEnvironmentOptions?.settings)
			}

Would respect the user-defined option for this

@capricorn86
Copy link
Owner

Instead of setting each option on the "window.happyDOM" property, I changed so that the object is sent into the Window constructor directly instead.

I did consider this but went the route of offering feedback to the consumer should the configuration be invalid. If you typo a key or use an invalid option, does this get bubbled up by happydom still?

I hope you are fine with the solution 🙂

It's simpler but at the cost of providing feedback to the consumer, and forces errorCapture to disabled.

The usage of errorCapture is actually my main motivator for adding this, as I have been trying to test error boundaries with react-router-dom so setting this to disabled works for my case.

I feel as though it should respect the setting specified in the config instead of being force disabled https://github.com/capricorn86/happy-dom/pull/1287/files#diff-ca9251b53f292fc136910984f98fa20367077cf2f260010ca5cf707d047a6a69R57

Flipping it to:

			settings: {
				errorCapture: BrowserErrorCaptureEnum.disabled,
				...(<IOptionalBrowserSettings>projectConfig.testEnvironmentOptions?.settings)
			}

Would respect the user-defined option for this

I think that it is probably better to add the validation of the settings in Window in that case, but it will affect performance. If we would add the validation in this package, it would require automated tests to ensure that it will not break as it is complex logic. Adding unit tests for this package would probably require some refactoring to separate integration tests from unit tests.

I put errorCapture after to ensure that it will not be set. The reason is that process level error capturing will not work at all (as it collides with Jest error capturing), and try/catch will capture some errors in tests, but Jest will not be able to detect them, leading to weird behaviors.

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