Skip to content

Assorted test-related improvements#2255

Merged
nirinchev merged 4 commits intomasterfrom
ni/enable-tests
Feb 19, 2021
Merged

Assorted test-related improvements#2255
nirinchev merged 4 commits intomasterfrom
ni/enable-tests

Conversation

@nirinchev
Copy link
Member

Description

This is mostly a cleanup PR that achieves the following:

  • Fixes OrderBy(...).OrderBy(...) query chains by having the last clause replace all preceding ones.
  • Reenables all previously disabled Windows tests. They seem to pass locally.
  • Reenable all previously explicit tests that have been fixed. Explicit tests that haven't been fixed are [Ignore]-d
  • Reenable some previously [Ignore]-d tests that have been fixed.
  • Throws RealmMigrationNeededException when opening a readonly file that uses an old core format.

TODO

  • Changelog entry

@nirinchev nirinchev self-assigned this Feb 17, 2021
@nirinchev nirinchev requested review from LaPeste and papafe February 17, 2021 12:13

protected override void CustomTearDown()
{
base.CustomTearDown();
Copy link
Contributor

Choose a reason for hiding this comment

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

If a Realm is disposed its sync session should be automatically disposed, so does this manual cleanup have the goal of speeding up the process of memory release or is there more to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

A sync session is independent of the Realm - it may certainly outlive the managed instance. The issue with the order here was that we were calling the base teardown method which disposes and deletes all Realm instances before disposing the session handles. This resulted in race conditions where if a session hasn't been disposed, the deletion would fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I misinterpreted this comment; I assume it simply meant that the user doesn't have to care to manually deallocating it.
Thank you for the explanation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the automatic lifespan management means that we'll create and terminate sessions based on certain heuristics/internal rules. Typically, this will mean that a session outlives its Realm so that it can upload any remaining changes. This has the unfortunate side effect that deleting a synchronized Realm, even after disposing it, is not exactly safe. This will hopefully be addressed by realm/realm-core#4424.

Copy link
Contributor

@LaPeste LaPeste left a comment

Choose a reason for hiding this comment

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

All seems fine to me.

Copy link
Contributor

@papafe papafe left a comment

Choose a reason for hiding this comment

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

Looks fine

@nirinchev nirinchev merged commit 2277bd5 into master Feb 19, 2021
@nirinchev nirinchev deleted the ni/enable-tests branch February 19, 2021 11:20
@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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants