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

test: use CHECK instead of EXPECT where necessary #44795

Closed
wants to merge 1 commit into from
Closed

test: use CHECK instead of EXPECT where necessary #44795

wants to merge 1 commit into from

Conversation

tniessen
Copy link
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

@tniessen
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 tniessen mentioned this pull request Sep 26, 2022
Merged
@nodejs-github-bot 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
RaisinTen approved these changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

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

@tniessen
Copy link
Member Author

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

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

This comment was marked as outdated.

Sign in to view
@RaisinTen
Copy link
Contributor

No idea, maybe @mhdawson knows?

@nodejs-github-bot
Copy link
Collaborator

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

@mhdawson
Copy link
Member

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

@mhdawson
Copy link
Member

mhdawson commented Sep 26, 2022 •
edited
Loading

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
Copy link
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
Copy link
Member

@tniessen what you do think of #44800 ?

@github-actions github-actions bot mentioned this pull request Sep 29, 2022
Open
16 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@tniessen
Copy link
Member Author

tniessen commented Oct 1, 2022

@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 tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 1, 2022
@tniessen
Copy link
Member Author

tniessen commented Oct 5, 2022

@mhdawson What do you think?

@mhdawson
Copy link
Member

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

mhdawson
mhdawson approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

mhdawson pushed a commit that referenced this pull request Oct 14, 2022
@tniessen @mhdawson
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

Landed in fcf27b1

@mhdawson mhdawson closed this Oct 14, 2022
@tniessen 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
@tniessen @RafaelGSS
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@tniessen @RafaelGSS
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@tniessen @danielleadams
test: use CHECK instead of EXPECT where necessary
aaa4bf2 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@tniessen @danielleadams
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@tniessen @danielleadams
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@mhdawson mhdawson mhdawson approved these changes

@ErickWendel ErickWendel ErickWendel approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@tniessen @nodejs-github-bot @RaisinTen @mhdawson @jasnell @ErickWendel