Skip to content

fix(material/core): sanity checks not running #23289

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

Merged
merged 1 commit into from
Aug 6, 2021

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Aug 1, 2021

Looks like when the granular sanity checks were introduced, some of the logic that determines whether a check should run wasn't written correctly which means that it isn't running at all.

These changes move the check logic into a separate method in order to make it less prone to mistakes.

Looks like when the granular sanity checks were introduced, some of the logic that determines whether a check should run wasn't written correctly which means that it isn't running at all.

These changes move the check logic into a separate method in order to make it less prone to mistakes.
@crisbeto crisbeto added 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 labels Aug 1, 2021
@crisbeto crisbeto requested a review from jelbourn August 1, 2021 12:06
@crisbeto crisbeto requested a review from devversion as a code owner August 1, 2021 12:06
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 1, 2021
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.

So, to clarify, these checks were never running? If so, I'm not sure if turning them on now would be considered breaking- I might make this PR target: major.

@crisbeto
Copy link
Member Author

crisbeto commented Aug 2, 2021

Not never, but I think it regressed in #16973. These are all console.warn so they probably won't break people's builds. Also if they were failing, people's apps would look broken.

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

@jelbourn jelbourn added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Aug 3, 2021
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Aug 3, 2021
@jelbourn
Copy link
Member

jelbourn commented Aug 3, 2021

Caretaker note: this might cause some Google teams to complain if they start seeing new warnings pop up in their logs

@andrewseguin andrewseguin merged commit f8778b5 into angular:master Aug 6, 2021
andrewseguin pushed a commit that referenced this pull request Aug 6, 2021
Looks like when the granular sanity checks were introduced, some of the logic that determines whether a check should run wasn't written correctly which means that it isn't running at all.

These changes move the check logic into a separate method in order to make it less prone to mistakes.

(cherry picked from commit f8778b5)
@eweap
Copy link

eweap commented Aug 13, 2021

Hi,
Since this PR was merged, I see a warning for each of my unit test during tests run. It makes the output totally unusable unless I "hacked" the console.warn method, which is not a good idea.

I will stick to 12.2.0 until an acceptable solution is found.

@jelbourn
Copy link
Member

@eweap these checks aren't supposed to run in test environments. Can you file an issue?

@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 Sep 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note 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
Development

Successfully merging this pull request may close these issues.

5 participants