Cwe 190 #1057
Merged
Cwe 190 #1057
Conversation
This file contains 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
|
@DaanHoogland, pretty good job, you created the test cases and removed the return null cases. |
|
@rafaelweingartner the cast was the reason for starting this. Coverity complained about a 32 bit shift on an integer (31 bit + sign) |
|
Got it @DaanHoogland. |
|
@rafaelweingartner sure, they're not needed but I'll add them for clarity. |
|
That is it. |
|
I was just re-checking the code, what about creating a test case for "netMaskFromCidr" method? |
|
@rafaelweingartner yes of course. you caught me pants down ;) |
| static long netMaskFromCidr(final long cidrSize) { | ||
| return ((long)0xffffffff) >> MAX_CIDR - cidrSize << MAX_CIDR - cidrSize; | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are tho static methods using "package" modifier? Are classes in the same package going to access it? If the answer is no, they can be made "private".
There was a problem hiding this comment.
for testing purpose (so yes)
There was a problem hiding this comment.
I think that it is fine letting the method with default modifier.
The point of a test case is that we can/should write tests for a single method. Therefore, if we want to test the method “getCidrSizeFromString”, we should write a test case for that specific method, and not for another one that uses it.
After we make sure that a method works fine, when we test another method (let’s say “cidrToLong”) that uses the first one already tested, we can use EasyMock/Mockito or another test suite to mock the method that we know works, this way we facilitate the writing of test cases.
DaanHoogland
force-pushed
the
CWE-190
branch
2 times, most recently
from
b0a8d87
to
02058b9
Compare
|
FYI: @DaanHoogland My tests show 29 test failures. I will retest to be sure, but please hold until this is done. Stuff like this: |
|
@remibergsma running them myself as well, now |
|
to many unexplained failures for such a small refactor. |
|
Hi @DaanHoogland, I have created a PR for your branch, so you can get those changes and reopen your PR. |
|
@rafaelweingartner thanks for picking this up. it is not a matter of giving up but of priorities and I didn't want to keep it open knowing there was work to do that I wouldn't look into. I'll have a look at your PR and run the integration suite against them. |
Changed the behavior of methods that use NetUtils.cidrToLong(String)
|
I understand why you closed the PR, but after a quick review, I thought that I could help you, so let’s see if now everything is ok |
|
@DaanHoogland @rafaelweingartner Much better now, just one failed test to resolve: Failed test: Details: |
|
thanks @remibergsma |
|
@DaanHoogland It could be a result of another failure. On master this test passes, so maybe you can run just this test yourself and look at the details? |
|
I was busy starting ;) |
|
:-) |
|
@remibergsma for me test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL fails. That is not the one you meant is it? I also saw the failure you had. |
|
@remibergsma, could you send me the stack trace of the test that failed? |
|
1057.network.results.txt |
|
LGTM based on these tests: Result: And: Result: I didn't test the actual feature. This just shows it didn't break anything else. |
asfgit
pushed a commit
that referenced
this pull request
Dec 6, 2015
Cwe 190coverity warnings of this type adressed. Some where dismissed and maybe with reason but it seemed possible to remove them and hence obligatory ;p * pr/1057: move back to original contract of isNetworksOverlap() Changed the behavior of methods that use NetUtils.cidrToLong(String) CWE-190 unit test for extremes of long netMaskFromCidr(long) CWE-190 netmask as long form cidr-size as method CID-1116482 cidrToLong cleanup of bitshift problem CID-1116483 cidr to netmask bitshifts guarded with casts CID-1116484 cast to long and use long as cidrsize type and simpel test CID-1116485: cast cidr during bit shifting and simple test included CID-1175714 casts before bit shift Signed-off-by: Daan Hoogland <daan@onecht.net>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
coverity warnings of this type adressed. Some where dismissed and maybe with reason but it seemed possible to remove them and hence obligatory ;p