test: fix issues reported by Coverity#8870
Closed
eugeneo wants to merge 0 commit intonodejs:masterfrom
eugeneo:coverty
Closed
test: fix issues reported by Coverity#8870eugeneo wants to merge 0 commit intonodejs:masterfrom eugeneo:coverty
eugeneo wants to merge 0 commit intonodejs:masterfrom
eugeneo:coverty
Conversation
eugeneo
changed the title
eugeneo
added
c++
Contributor
Author
|
Test failures are caused by parallel/test-tick-processor-builtin and parallel/test-tick-processor-unknown and do not seem to caused by this change, that only touches cctest executable. |
bnoordhuis
requested changes
Oct 1, 2016
test/cctest/test_inspector_socket.cc
Outdated
test/cctest/test_inspector_socket.cc
Outdated
Member
There was a problem hiding this comment.
Style nit: can you either call this SetFlag or set_flag? Ditto for markDone.
Contributor
Author
There was a problem hiding this comment.
Done (used set_flag and mark_done)
Member
|
LGTM once @bnoordhuis is happy with it. |
Contributor
Author
|
Thank you for the review. I uploaded a new version, please take another look. |
ofrobots
approved these changes
Oct 7, 2016
Contributor
|
LGTM |
Contributor
Author
|
@bnoordhuis I've implemented your suggestions, please take another look. |
bnoordhuis
approved these changes
Oct 12, 2016
test/cctest/test_inspector_socket.cc
Outdated
Member
There was a problem hiding this comment.
While you're here, maybe you can remove the extraneous parentheses around condition?
eugeneo
pushed a commit
that referenced
this pull request
Oct 13, 2016
Wrapped the timer into class to ensure it is cleaned up properly. PR-URL: #8870 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jasnell
pushed a commit
that referenced
this pull request
Oct 14, 2016
Wrapped the timer into class to ensure it is cleaned up properly. PR-URL: #8870 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins
pushed a commit
that referenced
this pull request
Nov 11, 2016
Wrapped the timer into class to ensure it is cleaned up properly. PR-URL: #8870 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
This is a refactoring of the inspector socket test case.
Description of change
Wrapped the timer into class to ensure it is cleaned up properly.
CC: @ofrobots