Skip to content

[ffigen] Fix flaky tests #1471

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

Merged
merged 5 commits into from
Aug 28, 2024
Merged

[ffigen] Fix flaky tests #1471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d35cb3c
Fix flaky tests
liamappelbe Aug 28, 2024
1837b7e
fmt
liamappelbe Aug 28, 2024
f8cf6b7
Remove @Retry
liamappelbe Aug 28, 2024
3967908
Attempted fix for global test
liamappelbe Aug 28, 2024
c3e9b11
Fix another flake
liamappelbe Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions pkgs/ffigen/test/native_objc_test/arc_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@
// Objective C support is only available on mac.
@TestOn('mac-os')

// This test is slightly flaky. Some of the ref counts are occasionally lower
// than expected, presumably due to the Dart wrapper objects being GC'd before
// the expect call.
@Retry(3)

import 'dart:ffi';
import 'dart:io';

Expand Down Expand Up @@ -97,6 +92,10 @@ void main() {
expect(objectRetainCount(obj2raw), 3);
expect(objectRetainCount(obj3raw), 2);

expect(obj1, isNotNull); // Force obj1 to stay in scope.
Copy link
Member

Choose a reason for hiding this comment

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

This would not be needed if the objects are Finalizable, but objects being Finalizable make them non-sharable.
So maybe we need the "StayAlive" marker interface @dcharkes!

In JNIgen I don't have a direct .pointer getter, instead users have to write .reference.pointer maybe that fixes the issue? I wonder if that works as .reference is used in the scope and is Finalizable but Pointer is not Finalizable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not worried about it. This is only an issue in these tests, where I'm explicitly testing the GC behavior. I don't think this will matter to users.

Copy link
Member

Choose a reason for hiding this comment

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

I am worried about it lol! We had this GC problem in package:jni that happened in user code randomly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. How did it manifest for users? I haven't been able to think of a situation where it would matter, other than tests like this.

expect(obj2, isNotNull); // Force obj2 to stay in scope.
expect(obj3, isNotNull); // Force obj3 to stay in scope.

return (obj1raw, obj2raw, obj3raw);
}

Expand Down Expand Up @@ -281,21 +280,19 @@ void main() {
final outerObjRaw = outerObj.pointer;
expect(objectRetainCount(outerObjRaw), 1);
final assignObjRaw = assignPropertiesInnerInner(counter, outerObj);
expect(objectRetainCount(assignObjRaw), 2);
doGC();
// assignObj has been cleaned up.
expect(counter.value, 1);
expect(objectRetainCount(assignObjRaw), 0);
expect(objectRetainCount(outerObjRaw), 1);
expect(outerObj, isNotNull); // Force outerObj to stay in scope.
return (outerObjRaw, assignObjRaw);
}

test('assign properties ref count correctly', () {
final counter = calloc<Int32>();
counter.value = 0;
final (outerObjRaw, assignObjRaw) = assignPropertiesInner(counter);
expect(objectRetainCount(assignObjRaw), 0);
expect(objectRetainCount(outerObjRaw), 1);
doGC();
expect(counter.value, 0);
expect(objectRetainCount(assignObjRaw), 0);
Expand Down Expand Up @@ -323,12 +320,12 @@ void main() {
final outerObjRaw = outerObj.pointer;
expect(objectRetainCount(outerObjRaw), 1);
final retainObjRaw = retainPropertiesInnerInner(counter, outerObj);
expect(objectRetainCount(retainObjRaw), 4);
doGC();
// retainObj is still around, because outerObj retains a reference to it.
expect(objectRetainCount(retainObjRaw), 2);
expect(objectRetainCount(outerObjRaw), 1);
expect(counter.value, 2);
expect(outerObj, isNotNull); // Force outerObj to stay in scope.
return (outerObjRaw, retainObjRaw);
}

Expand All @@ -339,8 +336,6 @@ void main() {
// need an autorelease pool.
final pool = lib.objc_autoreleasePoolPush();
final (outerObjRaw, retainObjRaw) = retainPropertiesInner(counter);
expect(objectRetainCount(retainObjRaw), 2);
expect(objectRetainCount(outerObjRaw), 1);
doGC();
expect(objectRetainCount(retainObjRaw), 1);
expect(objectRetainCount(outerObjRaw), 0);
Expand Down
4 changes: 4 additions & 0 deletions pkgs/ffigen/test/native_objc_test/global_native_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ void main() {
final obj2raw = obj2.pointer;
expect(objectRetainCount(obj2raw), 2); // obj2, and the global variable.
expect(objectRetainCount(obj1raw), 1); // Just obj1.
expect(obj1, isNotNull); // Force obj1 to stay in scope.
expect(obj2, isNotNull); // Force obj2 to stay in scope.

return (obj1raw, obj2raw);
}
Expand Down Expand Up @@ -79,6 +81,8 @@ void main() {
final blk2raw = blk2.pointer;
expect(blockRetainCount(blk2raw), 2); // blk2, and the global variable.
expect(blockRetainCount(blk1raw), 1); // Just blk1.
expect(blk1, isNotNull); // Force blk1 to stay in scope.
expect(blk2, isNotNull); // Force blk2 to stay in scope.

return (blk1raw, blk2raw);
}
Expand Down
4 changes: 4 additions & 0 deletions pkgs/ffigen/test/native_objc_test/global_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ void main() {
final obj2raw = obj2.pointer;
expect(objectRetainCount(obj2raw), 2); // obj2, and the global variable.
expect(objectRetainCount(obj1raw), 1); // Just obj1.
expect(obj1, isNotNull); // Force obj1 to stay in scope.
expect(obj2, isNotNull); // Force obj2 to stay in scope.

return (obj1raw, obj2raw);
}
Expand Down Expand Up @@ -80,6 +82,8 @@ void main() {
final blk2raw = blk2.pointer;
expect(blockRetainCount(blk2raw), 2); // blk2, and the global variable.
expect(blockRetainCount(blk1raw), 1); // Just blk1.
expect(blk1, isNotNull); // Force blk1 to stay in scope.
expect(blk2, isNotNull); // Force blk2 to stay in scope.

return (blk1raw, blk2raw);
}
Expand Down
17 changes: 6 additions & 11 deletions pkgs/ffigen/test/native_objc_test/ref_count_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@
// Objective C support is only available on mac.
@TestOn('mac-os')

// This test is slightly flaky. Some of the ref counts are occasionally lower
// than expected, presumably due to the Dart wrapper objects being GC'd before
// the expect call.
@Retry(3)

import 'dart:ffi';
import 'dart:io';

Expand Down Expand Up @@ -98,6 +93,10 @@ void main() {
expect(objectRetainCount(obj2raw), 3);
expect(objectRetainCount(obj3raw), 2);

expect(obj1, isNotNull); // Force obj1 to stay in scope.
expect(obj2, isNotNull); // Force obj2 to stay in scope.
expect(obj3, isNotNull); // Force obj3 to stay in scope.

return (obj1raw, obj2raw, obj3raw);
}

Expand Down Expand Up @@ -282,21 +281,19 @@ void main() {
final outerObjRaw = outerObj.pointer;
expect(objectRetainCount(outerObjRaw), 1);
final assignObjRaw = assignPropertiesInnerInner(counter, outerObj);
expect(objectRetainCount(assignObjRaw), 2);
doGC();
// assignObj has been cleaned up.
expect(counter.value, 1);
expect(objectRetainCount(assignObjRaw), 0);
expect(objectRetainCount(outerObjRaw), 1);
expect(outerObj, isNotNull); // Force outerObj to stay in scope.
return (outerObjRaw, assignObjRaw);
}

test('assign properties ref count correctly', () {
final counter = calloc<Int32>();
counter.value = 0;
final (outerObjRaw, assignObjRaw) = assignPropertiesInner(counter);
expect(objectRetainCount(assignObjRaw), 0);
expect(objectRetainCount(outerObjRaw), 1);
doGC();
expect(counter.value, 0);
expect(objectRetainCount(assignObjRaw), 0);
Expand Down Expand Up @@ -324,12 +321,12 @@ void main() {
final outerObjRaw = outerObj.pointer;
expect(objectRetainCount(outerObjRaw), 1);
final retainObjRaw = retainPropertiesInnerInner(counter, outerObj);
expect(objectRetainCount(retainObjRaw), 4);
doGC();
// retainObj is still around, because outerObj retains a reference to it.
expect(objectRetainCount(retainObjRaw), 2);
expect(objectRetainCount(outerObjRaw), 1);
expect(counter.value, 2);
expect(outerObj, isNotNull); // Force outerObj to stay in scope.
return (outerObjRaw, retainObjRaw);
}

Expand All @@ -340,8 +337,6 @@ void main() {
// need an autorelease pool.
final pool = lib.objc_autoreleasePoolPush();
final (outerObjRaw, retainObjRaw) = retainPropertiesInner(counter);
expect(objectRetainCount(retainObjRaw), 2);
expect(objectRetainCount(outerObjRaw), 1);
doGC();
expect(objectRetainCount(retainObjRaw), 1);
expect(objectRetainCount(outerObjRaw), 0);
Expand Down