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

Testing new datatypes #2797

Merged
merged 3 commits into from
Apr 16, 2020
Merged

Conversation

kneth
Copy link
Contributor

@kneth kneth commented Apr 6, 2020

What, How & Why?

Updating to latest core/sync/object store and adding more testing. Related to:

The Decimal128 tests will fail as outlined in realm/realm-core#3679

☑️ ToDos

  • 📝 Changelog entry
  • [ ] 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • [ ] 📝 Public documentation PR created or is not necessary
  • [ ] 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • [ ] typescript definitions file is updated
  • [ ] jsdoc files updated
  • [ ] Chrome debug API is updated if API is available on React Native

@kneth kneth self-assigned this Apr 6, 2020
@kneth kneth added the T-Test label Apr 6, 2020
@kneth kneth requested review from blagoev and kraenhansen April 6, 2020 08:43
Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

👍 with some comments

src/js_object_accessor.hpp Show resolved Hide resolved
@@ -1412,13 +1412,72 @@ module.exports = {

testCreateEmbeddedObjects: function() {
const realm = new Realm({schema: [schemas.ContactSchema, schemas.AddressSchema]});
realm.write(() => { // FIXME: This should not be required!
Copy link
Contributor

Choose a reason for hiding this comment

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

is this FIXME for now ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is fixed on the kneth/stitch-integration branch (by fixing Realm.clearTestState()).

for (let i = 0; i < numbers.length; i++) {
let d128 = objects[i]['decimalCol'];
TestCase.assertTrue(d128 instanceof Decimal128);
TestCase.assertEqual(d128.toString(), numbers[i].toUpperCase());
Copy link
Contributor

Choose a reason for hiding this comment

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

it will be really nice if we could add an equals(x: string): boolean to all decimals128 values which does the comparison with the correct casing. I looked at https://github.com/mongodb/js-bson/blob/master/lib/decimal128.js and strangely enough there is no such thing
Do you think we can add such method in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

envisioning this as d128.equals(string)


for (let i = 0; i < values.length; i++) {
let oid2 = objects[i]['id'];
TestCase.assertTrue(oid2 instanceof ObjectId, 'instaceof');
Copy link
Contributor

Choose a reason for hiding this comment

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

there is an TestCase.assertInstanceOf method

TestCase.assertEqual(oid2.toHexString(), oid1.toHexString());
values.forEach(v => {
let oid = ObjectId.createFromTime(v);
realm.write(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not have the realm.write wrap the forEach

tests/js/results-tests.js Show resolved Hide resolved
@kneth kneth merged commit 59dbc22 into kneth/stitch-integration Apr 16, 2020
@kneth kneth deleted the kneth/testing-new-datatypes branch April 16, 2020 09:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 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.

2 participants