Fix test for Dart 2.0 ints #9

keertip · 2018-01-09T21:57:14Z

No description provided.

nex3

It's probably a good idea to get @lrhn to review the test changes, since he wrote the tests in the first place.

nex3 · 2018-01-09T22:01:25Z

pubspec.yaml

@@ -1,5 +1,5 @@
 name: typed_data
-version: 1.1.5
+version: 1.1.6


Since this doesn't change any user-consumable code, can we just make it 1.1.6-dev?

bumped version and updated sdk constraints. does this look ok?

nex3 · 2018-01-09T22:04:18Z

test/typed_buffers_test.dart

+  0x7aaaaaaaaaaaaaaa,
+  0x7000000000000001,
+  0x7000000000000000,
+  0x7fffffffffffffff, //2^63


Isn't this 2^63 - 1?

yup, it is.

nex3 · 2018-01-09T22:04:46Z

test/typed_buffers_test.dart

-  expect(buffer.contains(min - 1), isFalse);
-  expect(buffer.contains(max + 1), isFalse);
+  // for signed 64 bit ints, min and max wrap around. min-1=max and max+1=min
+  if (bits == 64){


fixed, had to do without the formatter.

nex3 · 2018-01-09T22:06:41Z

test/typed_buffers_test.dart

  // Both values are in `samples`, but equality is performed without rounding.
-  expect(buffer.contains(min - 1), isFalse);
-  expect(buffer.contains(max + 1), isFalse);
+  // for signed 64 bit ints, min and max wrap around. min-1=max and max+1=min


Nit: Capitalization, punctuation.

alexmarkov · 2018-01-09T22:07:42Z

test/typed_buffers_test.dart

-  0x0ffffffffffffffff,
-  0xaaaaaaaaaaaaaaaa,
-  0x8000000000000001,
-  0x8000000000000000, // 2^63


Consider keeping 0x8000000000000000 value if possible, as it is a corner case which is worth checking.
With the new fixed-size wrap-around integers, 0x8000000000000000 == -0x8000000000000000 == min.

Works, added back.

nex3

Approving from a style perspective.

nex3 · 2018-01-09T22:17:41Z

CHANGELOG.md

@@ -1,3 +1,7 @@
+## 1.1.6
+
+* Fix tests to work with Dart 2.0 fixed size ints.


You can also remove this changelog entry now.

alexmarkov

LGTM

lrhn · 2018-01-10T10:36:57Z

test/typed_buffers_test.dart

-  0x10000000000000000, // 2^64
-  0x0ffffffffffffffff,
-  0xaaaaaaaaaaaaaaaa,
-  0x8000000000000001,


The 16-digit hex numerals (that is, all but the first two numerals) are all valid Dart 2 integer literals.
Hex literals are allowed to have values in the range -2^63 .. 2^64-1, with the upper ones being normalized to negative values. This allows you to write bit patterns efficiently.
So, only remove the first two, 0x10000000000000001 and 0x10000000000000000.

lrhn · 2018-01-10T10:45:25Z

test/typed_buffers_test.dart

+    // as signed ints.
+    expect(buffer.contains(min - 1), isTrue);
+  } else {
+    expect(buffer.contains(min - 1), isFalse);


I see no reason these tests can't run on 64-bit values too.
The min and max values might not really be min and max (they're 0 and -1 when represented as Int64), but the Uint64List.contains method will still do int.== to figure out if the argument is in the list, and max + 1 is 0, and that's definitely in the list.

So, all in all, I don't see a problem with keeping the test as-is (but maybe commenting that Uint64List acts exactly like Int64List because the int type cannot distinguish them.

The Uint64List tests will not work on JavaScript anyway because Uint64List doesn't exist.

For Uint64
min is 0, min-1 = -1
max is -1, max+1 = 0

and for Int64
min is -9223372036854775808, min-1 = 9223372036854775807
max is 9223372036854775807, max+1 = -9223372036854775808

so these lines

expect(buffer.contains(min - 1), isFalse); expect(buffer.contains(max + 1), isFalse);

fail. Since it wraps around, the values are contained in the buffer.

Ah, so contains didn't restrict the argument number to 64 bits before, but now it does. Makes sense. In that case, carry on!

lrhn · 2018-01-10T10:47:04Z

test/typed_buffers_test.dart

+  0x7aaaaaaaaaaaaaaa,
+  0x7000000000000001,
+  0x7000000000000000,
+  0x7fffffffffffffff, // 2^63-1


The numerals were in numerical order, so do keep that.

kevmoo · 2018-01-12T16:54:14Z

.travis.yml

 language: dart
 dart:
  - dev
-  - stable


Does this really need to drop stable? I don't see BigInt being used...

The except in the test returns different results on stable and dev, cause of difference in int64 behavior in the VM. Fixed it for dev16, but now it fails on stable.

Oh...hrm...

Couldn't we modify the tests so they work on both?

My concern: if we drop testing for stable, we should likely drop it in the pubspec, too.

But it'd be nice to avoid that since – AFAICT – there is no behavior change

With dev 16, min-1=max and max+1=min for int 64, so it wraps around. So now min-1 and max+1 are contained in the buffer, whereas previously they were not. This is a corner case, but it is a change in behavior. So change the pubspec?

Sure, change the pubspec to require the next version of the SDK,

@kevmoo ptal.

kevmoo

LGTM

* Fix test for Dart 2.0 ints * Update pubspec.yaml * Update typed_buffers_test.dart * Update typed_buffers_test.dart * Add 2^63 back to test data * Update CHANGELOG.md * Update typed_buffers_test.dart * Fix tests * run tests only on dev * Update pubspec

Fix test for Dart 2.0 ints

8e85e91

googlebot added the cla: yes label Jan 9, 2018

keertip requested a review from alexmarkov January 9, 2018 21:57

nex3 suggested changes Jan 9, 2018

View reviewed changes

keertip added 2 commits January 9, 2018 14:11

Update pubspec.yaml

f0ceb28

Update typed_buffers_test.dart

a38bed6

alexmarkov reviewed Jan 9, 2018

View reviewed changes

Update typed_buffers_test.dart

17dd960

nex3 approved these changes Jan 9, 2018

View reviewed changes

keertip and others added 2 commits January 9, 2018 14:20

Add 2^63 back to test data

1ef39b8

Update CHANGELOG.md

0f708e1

alexmarkov approved these changes Jan 9, 2018

View reviewed changes

keertip requested a review from lrhn January 9, 2018 22:23

lrhn reviewed Jan 10, 2018

View reviewed changes

Update typed_buffers_test.dart

a910496

lrhn mentioned this pull request Jan 12, 2018

Failing w/ Dart 2.0.0-dev.16 64-bit ints dart-lang/core#566

Closed

keertip added 3 commits January 12, 2018 08:35

Fix tests

3aff9c8

Merge branch 'tests' of github.com:dart-lang/typed_data into tests

cb6896a

run tests only on dev

58532e0

kevmoo reviewed Jan 12, 2018

View reviewed changes

Update pubspec

6ee3ab8

kevmoo approved these changes Jan 17, 2018

View reviewed changes

keertip merged commit 2d5ef0b into master Jan 17, 2018

kevmoo deleted the tests branch January 18, 2018 03:22

		@@ -1,3 +1,7 @@
		## 1.1.6

		* Fix tests to work with Dart 2.0 fixed size ints.

Uh oh!

Fix test for Dart 2.0 ints #9

Fix test for Dart 2.0 ints #9

Uh oh!

Conversation

keertip commented Jan 9, 2018

Uh oh!

nex3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nex3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexmarkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevmoo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants