FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: use CHECK instead of EXPECT where necessary by tniessen · Pull Request #44795 · nodejs/node · GitHub

/ node Public

test: use CHECK instead of EXPECT where necessary - #44795

Closed
tniessen wants to merge 1 commit into
nodejs:mainfrom
tniessen:test-crypto-clienthello-expect-check
ConversationCommits (1)ChecksFiles changed
Closed

test: use CHECK instead of EXPECT where necessary#44795
tniessen wants to merge 1 commit into
nodejs:mainfrom
tniessen:test-crypto-clienthello-expect-check

Conversation

tniessen commented Sep 26, 2022

Copy link
Copy Markdown
Member

GetPageSize() and OverrunGuardedBuffer currently use non-fatal EXPECT_* macros because GoogleTest does not allow the fatal variants ASSERT_* in non-void returning functions (i.e., in this file, nowhere outside of the TEST itself).

The EXPECT_* macros continue execution upon failure, but we really don't want that (and static analysis apparently does not like it either). Since we cannot use GoogleTest's ASSERT_* here, use our own CHECK_* instead of EXPECT_* outside of the TEST. Hopefully, this will finally pacify static analysis.

Refs: #44666

test: use CHECK instead of EXPECT where necessary
21df9e2 
GetPageSize() and OverrunGuardedBuffer currently use non-fatal EXPECT_*
macros because GoogleTest does not allow the fatal variants ASSERT_* in
non-void returning functions (i.e., in this file, nowhere outside of the
TEST itself).

The EXPECT_* macros continue execution upon failure, but we really don't
want that (and static analysis apparently does not like it either).
Since we cannot use GoogleTest's ASSERT_* here, use our own CHECK_*
instead of EXPECT_* outside of the TEST. Hopefully, this will finally
pacify static analysis.

Refs: nodejs#44666
tniessen requested a review from RaisinTen September 26, 2022 10:20
tniessen mentioned this pull request Sep 26, 2022
Merged
nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 26, 2022
RaisinTen approved these changes Sep 26, 2022
View reviewed changes

RaisinTen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, thanks! I had initially assumed that you ran coverity locally to verify the fix in the other PR. :)

tniessen commented Sep 26, 2022

Copy link
Copy Markdown
Member Author

Is that possible? If so, that would be really cool, but I never found a way to run it locally.

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2022

This comment was marked as outdated.

Sign in to view

RaisinTen commented Sep 26, 2022

Copy link
Copy Markdown
Member

No idea, maybe @mhdawson knows?

nodejs-github-bot commented Sep 26, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/46828/

mhdawson commented Sep 26, 2022

Copy link
Copy Markdown
Member

@RaisinTen unfortunately I don't know of any way to run Coverity locally.

mhdawson commented Sep 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Interestingly I was just looking at the recent coverity reports and was putting together a different PR. I have a feeling that coverity gets confused when we use macros that terminate (I believe I've marked a number of reported issues as false possitives in that case)

In this case I think the problem reported might not have been because we kept running but instead because type conversion might result in the check not working as expected. Just running the tests now and I'll paste the link to the PR once they run/pass.

mhdawson commented Sep 26, 2022

Copy link
Copy Markdown
Member

This is the fix I'd put together, #44800. It's really a pain that we can't run locally to check if we have resolved the issue or not.

This was referenced Sep 27, 2022
Open
Open

mhdawson commented Sep 28, 2022

Copy link
Copy Markdown
Member

@tniessen what you do think of #44800 ?

github-actions Bot mentioned this pull request Sep 29, 2022
Open
16 tasks

nodejs-github-bot commented Oct 1, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/46968/

tniessen commented Oct 1, 2022

Copy link
Copy Markdown
Member Author

@mhdawson Sorry about the delay. To be honest I don't quite see how #44800 solves the problem; I think it suffers from the same problem as GetPageSize() does without this PR. But I might be missing something -- coverity's output isn't always clear to me.

tniessen added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 1, 2022

tniessen commented Oct 5, 2022

Copy link
Copy Markdown
Member Author

@mhdawson What do you think?

mhdawson commented Oct 14, 2022

Copy link
Copy Markdown
Member

Not sure if this will fix the issue, but lets give it a try.

mhdawson approved these changes Oct 14, 2022
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mhdawson pushed a commit that referenced this pull request Oct 14, 2022
test: use CHECK instead of EXPECT where necessary
fcf27b1 
GetPageSize() and OverrunGuardedBuffer currently use non-fatal EXPECT_*
macros because GoogleTest does not allow the fatal variants ASSERT_* in
non-void returning functions (i.e., in this file, nowhere outside of the
TEST itself).

The EXPECT_* macros continue execution upon failure, but we really don't
want that (and static analysis apparently does not like it either).
Since we cannot use GoogleTest's ASSERT_* here, use our own CHECK_*
instead of EXPECT_* outside of the TEST. Hopefully, this will finally
pacify static analysis.

Refs: #44666

PR-URL: #44795
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>

mhdawson commented Oct 14, 2022

Copy link
Copy Markdown
Member

Landed in fcf27b1

mhdawson closed this Oct 14, 2022
tniessen deleted the test-crypto-clienthello-expect-check branch October 16, 2022 14:12
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
test: use CHECK instead of EXPECT where necessary
5f4fdb3 
GetPageSize() and OverrunGuardedBuffer currently use non-fatal EXPECT_*
macros because GoogleTest does not allow the fatal variants ASSERT_* in
non-void returning functions (i.e., in this file, nowhere outside of the
TEST itself).

The EXPECT_* macros continue execution upon failure, but we really don't
want that (and static analysis apparently does not like it either).
Since we cannot use GoogleTest's ASSERT_* here, use our own CHECK_*
instead of EXPECT_* outside of the TEST. Hopefully, this will finally
pacify static analysis.

Refs: #44666

PR-URL: #44795
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
RafaelGSS mentioned this pull request Nov 1, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
test: use CHECK instead of EXPECT where necessary
a618dc3 
GetPageSize() and OverrunGuardedBuffer currently use non-fatal EXPECT_*
macros because GoogleTest does not allow the fatal variants ASSERT_* in
non-void returning functions (i.e., in this file, nowhere outside of the
TEST itself).

The EXPECT_* macros continue execution upon failure, but we really don't
want that (and static analysis apparently does not like it either).
Since we cannot use GoogleTest's ASSERT_* here, use our own CHECK_*
instead of EXPECT_* outside of the TEST. Hopefully, this will finally
pacify static analysis.

Refs: #44666

PR-URL: #44795
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
test: use CHECK instead of EXPECT where necessary
5cf6100 
GetPageSize() and OverrunGuardedBuffer currently use non-fatal EXPECT_*
macros because GoogleTest does not allow the fatal variants ASSERT_* in
non-void returning functions (i.e., in this file, nowhere outside of the
TEST itself).

The EXPECT_* macros continue execution upon failure, but we really don't
want that (and static analysis apparently does not like it either).
Since we cannot use GoogleTest's ASSERT_* here, use our own CHECK_*
instead of EXPECT_* outside of the TEST. Hopefully, this will finally
pacify static analysis.

Refs: #44666

PR-URL: #44795
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
test: use CHECK instead of EXPECT where necessary
89a1c57 
GetPageSize() and OverrunGuardedBuffer currently use non-fatal EXPECT_*
macros because GoogleTest does not allow the fatal variants ASSERT_* in
non-void returning functions (i.e., in this file, nowhere outside of the
TEST itself).

The EXPECT_* macros continue execution upon failure, but we really don't
want that (and static analysis apparently does not like it either).
Since we cannot use GoogleTest's ASSERT_* here, use our own CHECK_*
instead of EXPECT_* outside of the TEST. Hopefully, this will finally
pacify static analysis.

Refs: #44666

PR-URL: #44795
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
ErickWendel ErickWendel approved these changes
RaisinTen RaisinTen approved these changes
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL