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

Enable all tests after bad rebase #5595

Merged
merged 38 commits into from
Apr 5, 2023
Merged

Enable all tests after bad rebase #5595

merged 38 commits into from
Apr 5, 2023

Conversation

kraenhansen
Copy link
Member

@kraenhansen kraenhansen commented Mar 20, 2023

What, How & Why?

When rebasing bindgen onto main we introduced a bad merge as tests on v11 was enumerated from a different file than that on bindgen, which resulted in many tests not being enabled.

This PR corrects this and fix all the remaining failures.

@kraenhansen kraenhansen self-assigned this Mar 20, 2023
@cla-bot cla-bot bot added the cla: yes label Mar 20, 2023
@kraenhansen kraenhansen force-pushed the kh/enable-all-tests branch 2 times, most recently from 56b8643 to 09b3179 Compare March 23, 2023 20:16
@kraenhansen kraenhansen mentioned this pull request Mar 29, 2023
1 task
@kraenhansen kraenhansen force-pushed the kh/enable-all-tests branch 9 times, most recently from cb2b868 to e42d41c Compare April 3, 2023 13:03
@kraenhansen kraenhansen marked this pull request as ready for review April 3, 2023 13:05
@@ -262,6 +263,19 @@ const CONNECTION_LISTENERS = new Listeners<ConnectionNotificationCallback, Liste
});

export class SyncSession {
private static instances = new IterableWeakRefs<SyncSession>();
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll be redoing this to use a simple Set<binding.WeakRef<SyncSession>> behind a ENABLE_CLEAN_TEST_STATE flag as per my conversations with @RedBeard0531.

@@ -83,7 +83,10 @@ export type TypeOptions = {
// "Only Realm instances are supported." (which should probably have been "RealmObject")
// instead of relying on the binding to throw.
export function mixedToBinding(realm: binding.Realm, value: unknown): binding.MixedArg {
if (typeof value === "undefined") {
if (typeof value === "string" || typeof value === "number" || typeof value === "boolean" || value === null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

There might be other types we should fast-track?

Copy link
Contributor

@elle-j elle-j Apr 4, 2023

Choose a reason for hiding this comment

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

This looks good, which other types were you thinking of in that case (for fast-track I mean)?

Copy link
Contributor

@elle-j elle-j left a comment

Choose a reason for hiding this comment

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

LGTM with some suggestions and questions 🙂

integration-tests/tests/src/tests.ts Outdated Show resolved Hide resolved
integration-tests/tests/src/tests/sync/sync-session.ts Outdated Show resolved Hide resolved
integration-tests/tests/src/utils/mock-function.ts Outdated Show resolved Hide resolved
packages/realm/src/ProgressRealmPromise.ts Show resolved Hide resolved
packages/realm/src/ProgressRealmPromise.ts Outdated Show resolved Hide resolved
packages/realm/src/PropertyMap.ts Outdated Show resolved Hide resolved
packages/realm/src/TypeHelpers.ts Outdated Show resolved Hide resolved
packages/realm/src/assert.ts Outdated Show resolved Hide resolved
@kraenhansen kraenhansen force-pushed the kh/enable-all-tests branch 2 times, most recently from 6e2530c to 6ed9ec1 Compare April 3, 2023 18:47
@@ -317,7 +317,7 @@ describe.skipIf(environment.missingServer, "User", () => {
.logIn(Realm.Credentials.emailPassword({ email: validEmail, password: validPassword }))
.catch((err) => {
expect(err.message).equals("invalid username/password");
expect(err.code).equals(50);
expect(err.code).equals(4349);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did the error code change? Was this a core upgrade?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that's a breaking change then :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Not as I see it. These are not documented, so technically not a part of our public API as per semantic versioning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm...true. Although if someone were to have code relying on this error code, it would need to be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

In any case, this doesn't block this PR from being merged. It was just a bit of a concern I had.

Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

LGTM!

kraenhansen and others added 24 commits April 5, 2023 17:36
This avoids firing listeners that gets added from other listeners
Co-authored-by: LJ <81748770+elle-j@users.noreply.github.com>
Co-authored-by: LJ <81748770+elle-j@users.noreply.github.com>
Co-authored-by: LJ <81748770+elle-j@users.noreply.github.com>
@kraenhansen kraenhansen merged commit 7600aea into main Apr 5, 2023
@kraenhansen kraenhansen deleted the kh/enable-all-tests branch April 5, 2023 16:47
@kraenhansen
Copy link
Member Author

While this didn't fix all issues when running on CI it's better than what we had. I'll adress the remaining failures in followup PRs.

papafe added a commit that referenced this pull request Apr 13, 2023
* main: (41 commits)
  allow useQuery to filter or sort a collection by using a callback (#5513)
  Fix `realm.create` types and 'requiredProperties' on Realm.Object constructor (#5697)
  Fix return type of `App.allUsers` (#5708)
  Removed renamed file
  Remove console.log
  Fix README instructions and bump version
  Bump template versions
  Update Templates (#5702)
  Moved "submodules" wireit task to "postinstall"
  Set up typedoc for realm-react, realm-web (#5709)
  Prepare for vNext (#5711)
  Prepare for 12.0.0-alpha.2 (#5707)
  Build iOS prebuilds in release by default (#5710)
  Use the event.sender as assignee when preparing release
  Updated prepare release workflow to print PR url in summary
  Fixing lint error
  fix the template app links (#5701)
  Expose `Sync` as named export (#5705)
  Enable all tests after bad rebase (#5595)
  Clear test state flag (#5689)
  ...

# Conflicts:
#	packages/realm/src/Realm.ts
@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.

3 participants