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.

Sign up for GitHub

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

Jump to bottom

fix(material/core): avoid running sanity checks on some test environments #23374

Merged
merged 1 commit into from Sep 21, 2021
Merged

fix(material/core): avoid running sanity checks on some test environments #23374

merged 1 commit into from Sep 21, 2021

Conversation

crisbeto
Copy link
Member

Fixes that the sanity checks were running on some testing environments like Jest.

These changes also move the test environment check into a centralized place so that it's easier to keep up to date.

Fixes #23365.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Aug 15, 2021
@crisbeto crisbeto requested a review from jelbourn August 15, 2021 10:15
@crisbeto crisbeto requested review from devversion and a team as code owners August 15, 2021 10:15
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 15, 2021
@josephperrott josephperrott removed the request for review from a team August 16, 2021 16:49
devversion
devversion approved these changes Aug 16, 2021
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM. Looks great to have this logic in a single place

src/material/core/common-behaviors/common-module.ts Outdated Show resolved Hide resolved
@leonardochaia
Copy link

Hoping that this get merged soon! our test builds logs are crowded with them and makes it hard to find other warnings.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker P2 The issue is important to a large percentage of users, with a workaround and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Aug 18, 2021
@jelbourn jelbourn removed the action: merge The PR is ready for merge by the caretaker label Aug 28, 2021
@crisbeto
fix(material/core): avoid running sanity checks on some test environm…
7ceb7ad 
…ents

Fixes that the sanity checks were running on some testing environments like Jest.

These changes also move the test environment check into a centralized place so that it's easier to keep up to date.

Fixes angular#23365.
@crisbeto
Copy link
Member Author

I've moved the utility into an underscored function.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Aug 29, 2021
jelbourn
jelbourn approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelfaith
Copy link

michaelfaith commented Sep 13, 2021 •
edited

@jelbourn Just wanted to check and see if this will be merged in soon?

@wagnermaciel wagnermaciel merged commit ca67623 into angular:master Sep 21, 2021
wagnermaciel pushed a commit that referenced this pull request Sep 21, 2021
@crisbeto @wagnermaciel
fix(material/core): avoid running sanity checks on some test environm…
01d8e89 
…ents (#23374)

Fixes that the sanity checks were running on some testing environments like Jest.

These changes also move the test environment check into a centralized place so that it's easier to keep up to date.

Fixes #23365.

(cherry picked from commit ca67623)
crisbeto added a commit to crisbeto/material2 that referenced this pull request Sep 24, 2021
@crisbeto
fix(material/core): sanity checks not disabled for node-based test en…
8e20836 
…vironments

In angular#23374 we expanded the logic that checks for test environments to cover Jest and Mocha. The problem is that we were checking against the `window` which won't work in a Node environment, because the global variables are attached to the `global` object, not the `window`, even though there may be a fake `window` declared by the test tooling.

These changes resolve the issue by first checking against `global` before falling back to `window`.

Fixes angular#23365.
@crisbeto crisbeto mentioned this pull request Sep 24, 2021
Merged
crisbeto added a commit to crisbeto/material2 that referenced this pull request Sep 24, 2021
@crisbeto
fix(material/core): sanity checks not disabled for node-based test en…
81e467d 
…vironments

In angular#23374 we expanded the logic that checks for test environments to cover Jest and Mocha. The problem is that we were checking against the `window` which won't work in a Node environment, because the global variables are attached to the `global` object, not the `window`, even though there may be a fake `window` declared by the test tooling.

These changes resolve the issue by first checking against `global` before falling back to `window`.

Fixes angular#23365.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Sep 24, 2021
@crisbeto
fix(material/core): sanity checks not disabled for node-based test en…
be6774f 
…vironments

In angular#23374 we expanded the logic that checks for test environments to cover Jest and Mocha. The problem is that we were checking against the `window` which won't work in a Node environment, because the global variables are attached to the `global` object, not the `window`, even though there may be a fake `window` declared by the test tooling.

These changes resolve the issue by first checking against `global` before falling back to `window`.

Fixes angular#23365.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Sep 24, 2021
@crisbeto
fix(material/core): sanity checks not disabled for node-based test en…
100028e 
…vironments

In angular#23374 we expanded the logic that checks for test environments to cover Jest and Mocha. The problem is that we were checking against the `window` which won't work in a Node environment, because the global variables are attached to the `global` object, not the `window`, even though there may be a fake `window` declared by the test tooling.

These changes resolve the issue by first checking against `global` before falling back to `window`.

Fixes angular#23365.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Sep 24, 2021
@crisbeto
fix(material/core): sanity checks not disabled for node-based test en…
52cba8d 
…vironments

In angular#23374 we expanded the logic that checks for test environments to cover Jest and Mocha. The problem is that we were checking against the `window` which won't work in a Node environment, because the global variables are attached to the `global` object, not the `window`, even though there may be a fake `window` declared by the test tooling.

These changes resolve the issue by first checking against `global` before falling back to `window`.

Fixes angular#23365.
mmalerba pushed a commit that referenced this pull request Sep 30, 2021
@crisbeto
fix(material/core): sanity checks not disabled for node-based test en…
4f6b9fd 
…vironments (#23636)

In #23374 we expanded the logic that checks for test environments to cover Jest and Mocha. The problem is that we were checking against the `window` which won't work in a Node environment, because the global variables are attached to the `global` object, not the `window`, even though there may be a fake `window` declared by the test tooling.

These changes resolve the issue by first checking against `global` before falling back to `window`.

Fixes #23365.
mmalerba pushed a commit that referenced this pull request Sep 30, 2021
@crisbeto @mmalerba
fix(material/core): sanity checks not disabled for node-based test en…
544423b 
…vironments (#23636)

In #23374 we expanded the logic that checks for test environments to cover Jest and Mocha. The problem is that we were checking against the `window` which won't work in a Node environment, because the global variables are attached to the `global` object, not the `window`, even though there may be a fake `window` declared by the test tooling.

These changes resolve the issue by first checking against `global` before falling back to `window`.

Fixes #23365.

(cherry picked from commit 4f6b9fd)
@sync-by-unito sync-by-unito bot mentioned this pull request Oct 1, 2021
Closed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@devversion devversion devversion approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: Sanity checks run in test environement
6 participants
@crisbeto @leonardochaia @michaelfaith @jelbourn @devversion @wagnermaciel