Skip to content
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

Fix boolean #557

Merged
merged 3 commits into from
May 9, 2022
Merged

Fix boolean #557

merged 3 commits into from
May 9, 2022

Conversation

blagoev
Copy link
Contributor

@blagoev blagoev commented May 8, 2022

Fixes boolean value persistence
fixes #474

@cla-bot cla-bot bot added the cla: yes label May 8, 2022
@coveralls
Copy link

coveralls commented May 8, 2022

Pull Request Test Coverage Report for Build 2293510920

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.677%

Totals Coverage Status
Change from base Build 2286108253: 0.0%
Covered Lines: 400
Relevant Lines: 427

💛 - Coveralls

@blagoev blagoev self-assigned this May 8, 2022
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

The changes seem fine to me, but I don't think the test is actually validating the fix - i.e. I expect the same test to have passed with the previous implementation due to the symmetric nature of the bug. A more robust test can be written using the to_json method on RealmObject and validating that the serialized json value matches expectations.

test/realm_object_test.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@desistefanova desistefanova left a comment

Choose a reason for hiding this comment

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

Agree with @nirinchev about items order could not be the same as the insertion order.

@blagoev
Copy link
Contributor Author

blagoev commented May 9, 2022

Insertion order is pretty much set in stone now. I can show you examples of tests depending on this behavior in every SDK.

@blagoev blagoev merged commit fef57c4 into master May 9, 2022
@blagoev blagoev deleted the fix-boolean branch May 9, 2022 10:39
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

type bool bug
4 participants