Ensure that Realm enums are accessible #5484

JacobOscarGunnarsson · 2023-02-23T08:47:27Z

Small tests to make sure that the issue in #3365 is resolved

JacobOscarGunnarsson · 2023-02-23T08:48:09Z

integration-tests/tests/src/tests/enums.ts

+  describe("ConnectionState", function () {
+    it("is accessible", function () {
+      expect(typeof ConnectionState === "object");
+      expect(typeof ConnectionState.Disconnected === "string");


Could be redundant to check all enum values, feel free to add thoughts about this.

I'd vote for keeping them (i.e. testing all values). Even though the issue mentioned asserting that the value is a string, we'd get more accurate tests if we compare it to the actual string value (e.g. "disconnected").

On a smaller note, perhaps we want to structure them more toward "natural language"? E.g.:

// This will pass for e.g. arrays as well. expect(typeof ConnectionState).to.equal("object"); // This will not pass for arrays. expect(ConnectionState).to.be.an("object"); expect(ConnectionState.Disconnected).to.equal("disconnected"); expect(ConnectionState.Connecting).to.equal("connecting"); // Fixed typo from `Connecting` to `Connected` expect(ConnectionState.Connected).to.equal("connected");

yeah went through this a bit too fast 🤦 using mocha as extensively as possible is what we've already done in test-migration-master so should aim for that! Thanks for pointing it out

Np 👍 (perhaps to.deep.equal() could test all those cases at once as well)

Could be redundant to check all enum values.

I'm probably feeling this a bit. If these tests fail it'll either be because we (hopefully consciously) changed the enums or the TS compiler is broken (in which case this will be the least of our worries). These tests are in risk of becoming outdated if we add new values to the enum.

The main reason we had these issues on master was because we declared types separately from implementation (in JS), so I probably don't feel the need for this to be a part of our test suite. But then again - they are written now, so they don't hurt too much to keep 😅

(and sorry for being late to the party here).

elle-j

Nice :D Got some suggestions 👍

elle-j · 2023-02-23T09:17:56Z

integration-tests/tests/src/tests/enums.ts

+  describe("ConnectionState", function () {
+    it("is accessible", function () {
+      expect(typeof ConnectionState === "object");
+      expect(typeof ConnectionState.Disconnected === "string");


I'd vote for keeping them (i.e. testing all values). Even though the issue mentioned asserting that the value is a string, we'd get more accurate tests if we compare it to the actual string value (e.g. "disconnected").

On a smaller note, perhaps we want to structure them more toward "natural language"? E.g.:

// This will pass for e.g. arrays as well. expect(typeof ConnectionState).to.equal("object"); // This will not pass for arrays. expect(ConnectionState).to.be.an("object"); expect(ConnectionState.Disconnected).to.equal("disconnected"); expect(ConnectionState.Connecting).to.equal("connecting"); // Fixed typo from `Connecting` to `Connected` expect(ConnectionState.Connected).to.equal("connected");

integration-tests/tests/src/tests/enums.ts

elle-j

😎

gagik

Nice, just a minor suggestion

gagik · 2023-02-23T09:41:05Z

integration-tests/tests/src/tests/enums.ts

-      expect(typeof NumericLogLevel.Trace === "number");
-      expect(typeof NumericLogLevel.Warn === "number");
+      expect(NumericLogLevel).to.be.an("object");
+      expect(NumericLogLevel.All).equals(0);


alternatively you could possibly (but I am not sure if this will work?) do a deep equal with an object i.e.

expect(NumericLogLevel).deep.equals({ All: 0, Trace: 1 ... });

This might be better as well in case in the future we add a new value to the enum, as it would cause the test to fail, which it wouldn't with the current approach. Same for other enums. Again, not sure if this will work how I think it'll work.

Just realized @elle-j had the same idea

I'll give it a shot, was a bit concerned that the syntax would make it less readable but the point with adding new values is very valid!

JacobOscarGunnarsson · 2023-02-23T10:15:46Z

I added the deep.equals, I'm a but torn on how it looks with the NumericLogLevel, but it might be worth it

elle-j · 2023-02-23T10:46:51Z

integration-tests/tests/src/tests/enums.ts

+      expect(NumericLogLevel).to.be.an("object");
+      expect(NumericLogLevel).to.deep.equals({


This first line shouldn't be needed when you're doing deep equals to an object. Btw, does it still work with having the sat the end of equal? (I'd vote for where you're using to.deep.equal()) :)

Yeah, most of these "to" "be" "equal/equals" are just meaningless syntactic sugar that does not change its behavior. So it could be expect(NumericLogLevel).deep.equals or expect(NumericLogLevel).to.deep.equal. I like both, I have generally been using the former for less use of this chaining but the latter does read nicer.

gagik · 2023-02-23T11:11:53Z

integration-tests/tests/src/tests/enums.ts

+  });
+  describe("NumericLogLevel", function () {
+    it("is accessible", function () {
+      expect(NumericLogLevel).to.deep.equal({


Just checked and we do use .deep.equals 161 trimes and .to.deep.equal 9 times... with that in mind I am leaning towards being consistent... sorry for leading you wrong way but I don't feel strongly about this if others don't

I like consistency so I appreciate that you lifted this :D

* bindgen: Implement getAllSyncSessions (#5492) Ensure that Realm enums are accessible (#5484) Apply suggestions from code review [skip ci] Small corrections [skip ci] Added changelog and final corrections Stub add test to validate that foreach throws on a dictionary (#5467) Using `RealmInsertionModel` on `Results#update` Updated "react-native" dev dep to 0.71.0 Bumped lower bound on our RN peer dependency [bindgen] SDK packaging (#5466) Adding "prebuild" and configuring it (#5447) add synthetic private brand fields to TS wrappers for C++ classes, and fix found bug import bindings directly rather than through internal Stub work

Ensure that enums are accessible

4f12a14

JacobOscarGunnarsson requested review from takameyer, kraenhansen, gagik and elle-j February 23, 2023 08:47

cla-bot bot added the cla: yes label Feb 23, 2023

JacobOscarGunnarsson commented Feb 23, 2023

View reviewed changes

elle-j reviewed Feb 23, 2023

View reviewed changes

use mocha more extensively, make tests more granular

a7e9e98

elle-j approved these changes Feb 23, 2023

View reviewed changes

gagik approved these changes Feb 23, 2023

View reviewed changes

Jacob Gunnarsson added 2 commits February 23, 2023 10:42

misspelling

94376c5

use deep equals on enums

62dbed8

JacobOscarGunnarsson requested review from gagik and elle-j February 23, 2023 10:14

elle-j reviewed Feb 23, 2023

View reviewed changes

remove unecessary object comparison, change equals to equal

aa75ccf

gagik reviewed Feb 23, 2023

View reviewed changes

change to deep.equals for consistency

33c3f45

kneth approved these changes Feb 24, 2023

View reviewed changes

JacobOscarGunnarsson merged commit f14917b into bindgen Feb 24, 2023

JacobOscarGunnarsson deleted the jg/realm_enums branch February 24, 2023 09:13

kraenhansen pushed a commit that referenced this pull request Mar 1, 2023

Ensure that Realm enums are accessible (#5484)

3cd44aa

kraenhansen pushed a commit that referenced this pull request Mar 3, 2023

Ensure that Realm enums are accessible (#5484)

e583169

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that Realm enums are accessible #5484

Ensure that Realm enums are accessible #5484

JacobOscarGunnarsson commented Feb 23, 2023

JacobOscarGunnarsson Feb 23, 2023

elle-j Feb 23, 2023 •

edited

Loading

JacobOscarGunnarsson Feb 23, 2023

elle-j Feb 23, 2023

kraenhansen Feb 24, 2023 •

edited

Loading

elle-j left a comment

elle-j Feb 23, 2023 •

edited

Loading

elle-j left a comment

gagik left a comment

gagik Feb 23, 2023

gagik Feb 23, 2023

JacobOscarGunnarsson Feb 23, 2023

JacobOscarGunnarsson commented Feb 23, 2023

elle-j Feb 23, 2023

gagik Feb 23, 2023

gagik Feb 23, 2023 •

edited

Loading

JacobOscarGunnarsson Feb 23, 2023

		expect(NumericLogLevel).to.be.an("object");
		expect(NumericLogLevel).to.deep.equals({

Ensure that Realm enums are accessible #5484

Ensure that Realm enums are accessible #5484

Conversation

JacobOscarGunnarsson commented Feb 23, 2023

Choose a reason for hiding this comment

elle-j Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

elle-j left a comment

Choose a reason for hiding this comment

elle-j Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

elle-j left a comment

Choose a reason for hiding this comment

gagik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobOscarGunnarsson commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagik Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j Feb 23, 2023 •

edited

Loading

kraenhansen Feb 24, 2023 •

edited

Loading

elle-j Feb 23, 2023 •

edited

Loading

gagik Feb 23, 2023 •

edited

Loading