-
Notifications
You must be signed in to change notification settings - Fork 90
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.
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
Realm Set support #1102
Realm Set support #1102
Conversation
fixed toSet() and added tests
added generator tests improve generator test failure logging
always use modifiable set for unmanaged realm set
Pull Request Test Coverage Report for Build 3967182069
💛 - Coveralls |
added tests
generator/test/error_test_data/unsupported_realm_set_with_default_values.expected
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good. Here are some small comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The helper methods and classes that are added for the purpose of the tests are a little bit confusing and the values of the sets are reinitialized too much. I prepared a suggestion how we can simplify the helper methods. I haven't touched the logic of the tests.
https://github.com/realm/realm-dart/blob/ds/realm_set_tests/test/realm_set_test.dart
} | ||
|
||
List<Object?> values(Type type) { | ||
return setByType(type).values; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will return new instances of Sets and sample values each time you want to reach a value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we have discussed this, but again. Do you see an issue with this? Is it a performance concern we should be addressing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also found it confusing when I read it - I think a more straightforward approach would be to create setByType(type)
in your test and then call .set
and .values
on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are combing multiple concepts into one. TestRealmSets
can be both unmanaged and managed and it contains RealmSet of different types so in order this to work in all cases one needs to go through the concrete instance of TestRealmSets
every time to get the correct set and value instances.
so each test needs to call
var set = testSet.setByType(type).set;
var values = testSet.values(type);
each time it needs to work with the correct data.
Also testSet.values
is just a convenience method for testSet.setByType(type).values
.
I am not getting an local var of the testSet.setByType(type)
result since then I will need three code lines each time to get the set
and values
from that local var. And I just don't care too much of the extra object created since this will get collected by the GC as soon as the method exits, since its a gen0 object.
In general I agree these auto generated tests in all test files in the project that we are creating are hard to work with. Especially amplified by the lack of good tooling support for such tests. But then I don't think we have an option not to auto generate these.
I will go with the tests that we have here and handle the feedback for the missing tests coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't checked all the tests, but getting testSet.setByType(type).set;
all the time really confused me. Probably because I expect that once I have an unmanaged object with field empty set
it will become managed after I add the main object to the realm. I might be wrong, but currently the new managed set is different instance than the unmanaged set.
For example, for the RealmList, if I create unmanaged team and players and then add the them to the realm, team.players collection is already managed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a super cursory review of the tests and the public API of the collection
|
||
set.add(values.first); | ||
expect(set.length, equals(1)); | ||
expect(set.elementAt(0), values.first); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not testing error conditions, such as elementAt(1)
, elementAt(-1)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be testing these. At least not here. Unmanged RealmSet is just an implementation of Dart Set. Did you mean to test these somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have tests for these in the managed case in the elementAt
tests
expect(set.length, equals(1)); | ||
expect(set.contains(values.first), true); | ||
|
||
set.remove(values.first); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this test the return value of remove
? Also, should we have a test for removing a non-existent item?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above for unmanaged add
final set = testSet.setByType(type).set; | ||
final values = testSet.values(type); | ||
|
||
set.add(values.first); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be testing the return value of add
? Also, should we test adding a duplicate item?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the unmanaged set implementation which is just inherited from a Dart set. I don't think we should be testing that here.
I have changed the managed test to test for the add
result
@nirinchev Thanks for the review. I will try addressing your suggestions above |
Please, ignore this comment: I checked it in details and it works as it is expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder to add realm_set_test.dart
"flutter\realm_flutter\tests\test_driver\realm_test.dart".
Good catch. Will add this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: unmanaged set tests - I think we should treat the unmanaged set implementation as an implementation detail and not rely that it'll never change, so adding some tests for the behavior of add/remove/elementAt makes sense. We've had issues on the .NET SDK with inconsistent behavior between managed and unmanaged sets, especially with RealmValue
containing different value types, but the same value (e.g. 0 integer, 0 double, and 0 decimal). I don't know if that'll be an issue for dart, but maybe worth adding a test.
bool remove(covariant T value); //Note: explicitly overriding remove() to change parameter type | ||
|
||
/// Clear the set and if it is a set of Realm objects, delete all realm objects from the realm. | ||
void deleteAll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird - does it really delete the objects from Realm? I don't believe such an API belongs on the set - we don't have it for list and I don't think we expose a similar API in any of the other SDKs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the test expectations show that objects are indeed deleted from the realm and this is exposed in the C-API.
I don't see why not keeping it. Perhaps we can extends List to support it as well later. I think it's a good usability improvement. Don't know how much we should sync on this. Perhaps we could leave it and see if it is requested for the other SDKs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to have it - generally, we want to expose operations that mutate the Realm on the Realm itself. We already have Realm.deleteMany
- if we want to optimize performance we could detect that the iterable is a RealmSet and call the C API to delete all elements. But having both clear
and deleteAll
on RealmSet is not good design.
We really redirect all UnmanagedSet operations to a base Set class which works with |
fixes #779
This also fixes the generator tests to output the expected and the actual files. Also adds a launch config to debug the generator in VS Code.