You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@plc4x.apache.org by cd...@apache.org on 2018/02/21 14:55:13 UTC
[incubator-plc4x] branch master updated: Fix the ByteValueTest
This is an automated email from the ASF dual-hosted git repository.
cdutz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-plc4x.git
The following commit(s) were added to refs/heads/master by this push:
new 418677d Fix the ByteValueTest
418677d is described below
commit 418677dc4ae9d9fb25c7f249587d07711578fc26
Author: Christofer Dutz <ch...@c-ware.de>
AuthorDate: Wed Feb 21 15:55:09 2018 +0100
Fix the ByteValueTest
---
.../src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java b/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
index 3a8b7b8..16abdf3 100644
--- a/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
+++ b/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
@@ -65,7 +65,7 @@ public class ByteValueTest {
@Test(expected = IllegalArgumentException.class)
public void checkUnsignedBoundsBigTooBig() {
- ByteValue.checkUnsignedBounds(new BigInteger(Long.toString(upperBound)), 4);
+ ByteValue.checkUnsignedBounds(new BigInteger(Long.toString(upperBound)).add(BigInteger.ONE), 4);
}
@Test
--
To stop receiving notification emails like this one, please contact
cdutz@apache.org.
Re: [incubator-plc4x] branch master updated: Fix the ByteValueTest
Posted by Sebastian Rühl <se...@googlemail.com>.
I take a look at it.
Regarding breaking build: somehow our pipeline implementation handles a test fail a failure rather than unstable.
So from my POV it would be a +1 on fixing obvious small bugs as soon as you find them, but when we find a bug with a test that we can’t immediately fix I think it would be good to check in the failing test anyway because then we a required to take care of the problem.
Sebastian
> Am 22.02.2018 um 08:11 schrieb Christofer Dutz <ch...@c-ware.de>:
>
> Hi Justin.
>
> Ok ... if the max value is outside the bounds and the code should have thrown an error, then I apologize.
> I just saw that the value being passed in was the same as the max allowed upper bound and thought the test was wrong.
>
> Sorry for double breaking __
>
> Maybe Sebastian can have a look at this.
>
> But if you find something like this, wouldn't it be better to fix the broken code or at least leave a comment in the test that is guaranteed to break?
>
> Chris
>
>
> Am 21.02.18, 22:30 schrieb "Justin Mclean" <ju...@classsoftware.com>:
>
> Hi,
>
> Perhaps my email wasn’t clear. The error is not in the test the error is in the code the test was showing that. :-)
>
> Thanks,
> Justin
>
>> On 22 Feb 2018, at 1:55 am, cdutz@apache.org wrote:
>>
>> This is an automated email from the ASF dual-hosted git repository.
>>
>> cdutz pushed a commit to branch master
>> in repository https://gitbox.apache.org/repos/asf/incubator-plc4x.git
>>
>>
>> The following commit(s) were added to refs/heads/master by this push:
>> new 418677d Fix the ByteValueTest
>> 418677d is described below
>>
>> commit 418677dc4ae9d9fb25c7f249587d07711578fc26
>> Author: Christofer Dutz <ch...@c-ware.de>
>> AuthorDate: Wed Feb 21 15:55:09 2018 +0100
>>
>> Fix the ByteValueTest
>> ---
>> .../src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java b/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
>> index 3a8b7b8..16abdf3 100644
>> --- a/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
>> +++ b/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
>> @@ -65,7 +65,7 @@ public class ByteValueTest {
>>
>> @Test(expected = IllegalArgumentException.class)
>> public void checkUnsignedBoundsBigTooBig() {
>> - ByteValue.checkUnsignedBounds(new BigInteger(Long.toString(upperBound)), 4);
>> + ByteValue.checkUnsignedBounds(new BigInteger(Long.toString(upperBound)).add(BigInteger.ONE), 4);
>> }
>>
>> @Test
>>
>> --
>> To stop receiving notification emails like this one, please contact
>> cdutz@apache.org.
>
>
>
Re: [incubator-plc4x] branch master updated: Fix the ByteValueTest
Posted by Sebastian Rühl <se...@googlemail.com>.
Hi,
You are right: with 0x1_00_00 we ensure that we fail at the right boundary and not at something bigger than 0xFF_FF.
I’ll fix that.
Thanks, Sebastian
> Am 22.02.2018 um 12:06 schrieb Justin Mclean <ju...@me.com>:
>
> Hi,
>
>> I extended the test with duplicated hex variants to better visualize the boundaries:
>> @Test
>> public void checkUnsignedBoundsLongHex() {
>> // Hex representation to visualize valid bounds in bytes
>> ByteValue.checkUnsignedBounds(0x0_00_00, 2);
>> ByteValue.checkUnsignedBounds(0x0_FF_FF, 2);
>> }
>> @Test(expected = IllegalArgumentException.class)
>> public void checkUnsignedBoundsLongTooBigHex() {
>> ByteValue.checkUnsignedBounds(0x1_FF_FF, 2);
>> }
>
> Should that be 0x1_00_00 rather than 0x1_FF_FF? i.e. 0x0_FF_FF +1?
>
> Thanks,
> Justin
Re: [incubator-plc4x] branch master updated: Fix the ByteValueTest
Posted by Justin Mclean <ju...@me.com>.
Hi,
> I extended the test with duplicated hex variants to better visualize the boundaries:
> @Test
> public void checkUnsignedBoundsLongHex() {
> // Hex representation to visualize valid bounds in bytes
> ByteValue.checkUnsignedBounds(0x0_00_00, 2);
> ByteValue.checkUnsignedBounds(0x0_FF_FF, 2);
> }
> @Test(expected = IllegalArgumentException.class)
> public void checkUnsignedBoundsLongTooBigHex() {
> ByteValue.checkUnsignedBounds(0x1_FF_FF, 2);
> }
Should that be 0x1_00_00 rather than 0x1_FF_FF? i.e. 0x0_FF_FF +1?
Thanks,
Justin
Re: [incubator-plc4x] branch master updated: Fix the ByteValueTest
Posted by Sebastian Rühl <se...@googlemail.com>.
Hi,
I extended the test with duplicated hex variants to better visualize the boundaries:
@Test
public void checkUnsignedBoundsLongHex() {
// Hex representation to visualize valid bounds in bytes
ByteValue.checkUnsignedBounds(0x0_00_00, 2);
ByteValue.checkUnsignedBounds(0x0_FF_FF, 2);
}
@Test(expected = IllegalArgumentException.class)
public void checkUnsignedBoundsLongTooBigHex() {
ByteValue.checkUnsignedBounds(0x1_FF_FF, 2);
}
And the same for BigInteger.
Sebastian
> Am 22.02.2018 um 11:39 schrieb Justin Mclean <ju...@me.com>:
>
> Hi,
>
>> Ok ... if the max value is outside the bounds and the code should have thrown an error, then I apologize.
>
> No issue. Looking again at it I’m still not sure which bit or code or test is wrong or perhaps neither(?) but I think it should be consistent. Perhaps Sebastian has a better idea?
>
>> But if you find something like this, wouldn't it be better to fix the broken code or at least leave a comment in the test that is guaranteed to break?
>
> I wasn't 100% sure which method had the incorrect boundary condition.
>
> Thanks,
> Justin
>
>
Re: [incubator-plc4x] branch master updated: Fix the ByteValueTest
Posted by Justin Mclean <ju...@me.com>.
Hi,
> Ok ... if the max value is outside the bounds and the code should have thrown an error, then I apologize.
No issue. Looking again at it I’m still not sure which bit or code or test is wrong or perhaps neither(?) but I think it should be consistent. Perhaps Sebastian has a better idea?
> But if you find something like this, wouldn't it be better to fix the broken code or at least leave a comment in the test that is guaranteed to break?
I wasn't 100% sure which method had the incorrect boundary condition.
Thanks,
Justin
Re: [incubator-plc4x] branch master updated: Fix the ByteValueTest
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi Justin.
Ok ... if the max value is outside the bounds and the code should have thrown an error, then I apologize.
I just saw that the value being passed in was the same as the max allowed upper bound and thought the test was wrong.
Sorry for double breaking __
Maybe Sebastian can have a look at this.
But if you find something like this, wouldn't it be better to fix the broken code or at least leave a comment in the test that is guaranteed to break?
Chris
Am 21.02.18, 22:30 schrieb "Justin Mclean" <ju...@classsoftware.com>:
Hi,
Perhaps my email wasn’t clear. The error is not in the test the error is in the code the test was showing that. :-)
Thanks,
Justin
> On 22 Feb 2018, at 1:55 am, cdutz@apache.org wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> cdutz pushed a commit to branch master
> in repository https://gitbox.apache.org/repos/asf/incubator-plc4x.git
>
>
> The following commit(s) were added to refs/heads/master by this push:
> new 418677d Fix the ByteValueTest
> 418677d is described below
>
> commit 418677dc4ae9d9fb25c7f249587d07711578fc26
> Author: Christofer Dutz <ch...@c-ware.de>
> AuthorDate: Wed Feb 21 15:55:09 2018 +0100
>
> Fix the ByteValueTest
> ---
> .../src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java b/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
> index 3a8b7b8..16abdf3 100644
> --- a/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
> +++ b/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
> @@ -65,7 +65,7 @@ public class ByteValueTest {
>
> @Test(expected = IllegalArgumentException.class)
> public void checkUnsignedBoundsBigTooBig() {
> - ByteValue.checkUnsignedBounds(new BigInteger(Long.toString(upperBound)), 4);
> + ByteValue.checkUnsignedBounds(new BigInteger(Long.toString(upperBound)).add(BigInteger.ONE), 4);
> }
>
> @Test
>
> --
> To stop receiving notification emails like this one, please contact
> cdutz@apache.org.
Re: [incubator-plc4x] branch master updated: Fix the ByteValueTest
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
Perhaps my email wasn’t clear. The error is not in the test the error is in the code the test was showing that. :-)
Thanks,
Justin
> On 22 Feb 2018, at 1:55 am, cdutz@apache.org wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> cdutz pushed a commit to branch master
> in repository https://gitbox.apache.org/repos/asf/incubator-plc4x.git
>
>
> The following commit(s) were added to refs/heads/master by this push:
> new 418677d Fix the ByteValueTest
> 418677d is described below
>
> commit 418677dc4ae9d9fb25c7f249587d07711578fc26
> Author: Christofer Dutz <ch...@c-ware.de>
> AuthorDate: Wed Feb 21 15:55:09 2018 +0100
>
> Fix the ByteValueTest
> ---
> .../src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java b/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
> index 3a8b7b8..16abdf3 100644
> --- a/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
> +++ b/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/api/util/ByteValueTest.java
> @@ -65,7 +65,7 @@ public class ByteValueTest {
>
> @Test(expected = IllegalArgumentException.class)
> public void checkUnsignedBoundsBigTooBig() {
> - ByteValue.checkUnsignedBounds(new BigInteger(Long.toString(upperBound)), 4);
> + ByteValue.checkUnsignedBounds(new BigInteger(Long.toString(upperBound)).add(BigInteger.ONE), 4);
> }
>
> @Test
>
> --
> To stop receiving notification emails like this one, please contact
> cdutz@apache.org.