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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(repository-tests): move relation tests to repository-tests package #3538

Merged
merged 1 commit into from Sep 3, 2019

Conversation

@agnes512
Copy link
Contributor

commented Aug 12, 2019

resolves #3442

  • Move these tests to repository-tests package so that they would be tested against real databases such as MongoDB and MySQL.
  • Introduce a new type mixedType for different id types usage
  • Refactor Repository classes so that they are inherited from CrudRepositoryCtor.

relation.factory.integration.ts is split into has-many.factory.integration.ts and belongs-to.factory.integration.ts

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@nabdelgadir nabdelgadir force-pushed the refactor-tests branch 2 times, most recently from c6feb46 to 7710fba Aug 12, 2019

@nabdelgadir nabdelgadir force-pushed the refactor-tests branch 2 times, most recently from d42454a to 975fc50 Aug 14, 2019

@agnes512 agnes512 changed the title test: add files refactor(repository-tests): move relation tests to repository-tests package Aug 14, 2019

@agnes512 agnes512 marked this pull request as ready for review Aug 14, 2019

@agnes512 agnes512 requested review from bajtos and raymondfeng as code owners Aug 14, 2019

@agnes512

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@raymondfeng I don't know if we need to export these tests as functions like what we did in this file.
We keep them as they are because they run fine.

^ nvm we are gonna change it

@nabdelgadir nabdelgadir force-pushed the refactor-tests branch 2 times, most recently from 791f572 to f551ade Aug 14, 2019

@nabdelgadir nabdelgadir changed the title refactor(repository-tests): move relation tests to repository-tests package [WIP] refactor(repository-tests): move relation tests to repository-tests package Aug 14, 2019

@nabdelgadir nabdelgadir force-pushed the refactor-tests branch 3 times, most recently from 7786f05 to a94bb14 Aug 14, 2019

@agnes512 agnes512 removed request for raymondfeng and bajtos Aug 15, 2019

@nabdelgadir nabdelgadir force-pushed the refactor-tests branch 8 times, most recently from 5800d99 to 9fa74e5 Aug 15, 2019

@bajtos
Copy link
Member

left a comment

Thank you for taking this non-trivial task!

Based on what I see in the code, I think there are few tricky areas that may require a bit of research:

  • How to define model classes to take into account features.idType
  • How to work with repository classes and relation repository factories/accessors.

I feel that moving all tests in one pull request is not very practical, as there is too much code involved. I am proposing to split this effort into smaller chunks that will make it easier to iterate on the test design.

Personally, I would start with a very small piece and pick a single relation type (BelongsTo or HasMany) and a single test type (e.g. integration tests for the repository factory).

"debug": "^4.1.1"
"debug": "^4.1.1",
"@loopback/context": "^1.21.1",
"@loopback/core": "^1.9.0"

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 16, 2019

Member

I find it suspicious that we need to add these new dependencies. I quickly skimmed through the code and don't see any place using them. Can you double check please?

Ideally, there should be no changes in packages/repository-tests/package.json and packages/repository-tests/package-lock.json needed.

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Aug 16, 2019

Contributor

We used '@loopback/context' in our repository fixtures: import {Getter, inject} from '@loopback/context';, but I don't think we use @loopback/core anywhere so I'll remove it

packages/repository-tests/src/relations-test-suite.ts Outdated Show resolved Hide resolved
async () => customerRepo,
async () => shipmentRepo,
);
customerRepo = new repositoryClass(Customer, db) as CustomerRepository;

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 16, 2019

Member

This isn't going to work.

Your CustomerRepository is always extending DefaultCrudRepository, ignoring repositoryClass constructor provided from outside.

When you call new repositoryClass, an instance of repositoryClass (typically DefaultCrudRepository) is created. The constructor of CustomerRepository is not called. As a result, the repository instance won't have any orders property as defined in CustomerRepository.

I see two options how to move forward:

(1)
Move the code creating repositories into a factory function, accepting repositoryClass as an argument.

For example:

export function createCustomerRepository(repositoryClass: /*insert type*/) {
  return class CustomerRepository extends repositoryClass<Customer, /*how do we specify id type here? */> {
    // ...
  }
}

(2)
Don't create model-specific repository classes, do everything from outside in the test.

async function givenBoundCrudRepositories(db: juggler.DataSource) {
  orderRepo = new repositoryClass(Order, db);
  // option 1: create variable instead of a property
  orderCustomer = createBelongsToAccessor(
    /* lookup relation definition */, 
    async () => customerRepo,
    orderRepo,
  );

  // option 2: define a property - this requires `orderRepo` to be cast to something like 
  // `repositoryClass & {shipment: BelongsToAccessor<Shipment, typeof Order.prototype.id>;}`
  orderRepo.shipment = createBelongsToAccessor(
    /* lookup relation definition */, 
    async () => shipmentRepo,
    orderRepo
  );

  return {orderRepo, orderCustomer};
}

This comment has been minimized.

Copy link
@agnes512

agnes512 Aug 16, 2019

Author Contributor

We were trying to pass db to the CustomerRepository in here. I am not sure if it doesn't run on mongo case we solved some mongo failures from this file before.

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Aug 20, 2019

Contributor

We created a helper file to get the repositories. What do you think?

@nabdelgadir nabdelgadir force-pushed the refactor-tests branch 2 times, most recently from def69c1 to ec41e56 Aug 16, 2019

@nabdelgadir nabdelgadir force-pushed the refactor-tests branch from 708a1c7 to b267a7b Aug 19, 2019

@nabdelgadir nabdelgadir changed the title [WIP] refactor(repository-tests): move relation tests to repository-tests package refactor(repository-tests): move relation tests to repository-tests package Aug 19, 2019

@nabdelgadir nabdelgadir force-pushed the refactor-tests branch from b267a7b to bda386c Aug 19, 2019

@nabdelgadir nabdelgadir requested a review from bajtos Aug 19, 2019

@nabdelgadir nabdelgadir force-pushed the refactor-tests branch 2 times, most recently from dc8782a to 299fd3f Aug 20, 2019

@jannyHou
Copy link
Contributor

left a comment

Great effort! LGTM in general, left one comment. I assume the test files are copied from the old ones, so didn't review them. The CI tests are failing and seem related, please fix them before merge :)

@property({
type: 'string',
})
street: string;
@property({
type: 'string',
id: true,
default: '12345',

This comment has been minimized.

Copy link
@jannyHou

jannyHou Aug 21, 2019

Contributor

hmm, why do we need these default values?

This comment has been minimized.

Copy link
@agnes512

agnes512 Aug 21, 2019

Author Contributor

If we don't set the default value, MySQL somehow would have undefined instead of null

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Aug 21, 2019

Contributor

The CI tests are failing and seem related

They were just timeout failures. They're gone now.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Aug 22, 2019

Contributor

MySQL somehow would have undefined instead of null

This behavior sounds reasonable to me, 馃 , why is it a problem? Some tests expect null instead of undefined? Or any API calls will be broken?

This comment has been minimized.

Copy link
@agnes512

agnes512 Aug 22, 2019

Author Contributor

If nothing is provided then loopback uses undefined but mysql uses null. And if we use null in lb4, it's not the same thing as it's in MySQL. That's why we use these default values.

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 26, 2019

Member

It's a well-know problem (feature?) that NoSQL databases like memory and MongoDB represent missing values as undefined, while SQL databases use NULL. We have features.emptyValue to use in tests to deal with that difference.

Please remove default fields from the property definitions and modify the checks to use feature.emptyValue instead of the default or undefined.

See e.g. this test for inspiration:

expect(toJSON(found)).to.deepEqual(
toJSON({
id: created.id,
name: 'new name',
description: features.emptyValue,
}),
);
});

@bajtos bajtos referenced this pull request Aug 23, 2019
7 of 7 tasks complete
@bajtos
Copy link
Member

left a comment

Nice! I see you are making good progress here, discovering subtle differences in database-specific behavior. I find certain parts of the proposed changes problematic, let's do few more iterations to improve them.

// use strictObjectIDCoercion here to make sure mongo's happy
@model({
settings: {
strictObjectIDCoercion: true,

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 26, 2019

Member

Configuring connector-specific settings in the shared test suite creates unwanted coupling between the generic test suite (that should be connector-independent) and individual connectors. It makes it difficult for individual connectors to tweak these settings, because the change must be made in the shared test suite - this is even more difficult for 3rd party connectors. Let's find a better solution please.

I am proposing to introduce a new features flag allowing each connector to provide additional model settings that should be used by all models. Example usage here:

@model({
  settings: features.modelSettings,
})
class Order extends Entity {
  // ...
}

However! I believe the MongoDB connector allows users to enable strictObjectIDCoercion at dataSource-level, for all models connected to that datasource. I prefer to enable strictObjectIDCoercion that way (see MONGODB_CONFIG) instead of introducing a new CrudFeature flag.

I'd also like to better understand why is it necessary to enable strictObjectIDCoercion? What is happening under the hood?

If strictObjectIDCoercion is necessary to make relations work, then we need to make this very clear in our documentation! Ideally, we should modify lb4 datasource generator to enable that flag in every datasource that's using the MongoDB connector. Docs & CLI updates are out of scope of this pull request, we should open a new issue to keep track of those changes. Ideally, that issue should explain why is that flag necessary (what is happening under the hood).

Very loosely related: In the future, I'd like us to rework MongoDB connector so that ObjectID type is never leaked outside of the connector, so that the Models and any user-facing code is using string instead. See #3456

This comment has been minimized.

Copy link
@agnes512

agnes512 Aug 27, 2019

Author Contributor

We use strictObjectIDCoercion here cause I checked these 2 issues: Mongo fails to find user by id, Document strictObjectIDCorecion flag. And also since we were using string, I thought it might help to deal with ObjectId.
But I don't think it works as expected. It will be removed.

@property({
type: 'string',
})
street: string;
@property({
type: 'string',
id: true,
default: '12345',

This comment has been minimized.

Copy link
@bajtos

bajtos Aug 26, 2019

Member

It's a well-know problem (feature?) that NoSQL databases like memory and MongoDB represent missing values as undefined, while SQL databases use NULL. We have features.emptyValue to use in tests to deal with that difference.

Please remove default fields from the property definitions and modify the checks to use feature.emptyValue instead of the default or undefined.

See e.g. this test for inspiration:

expect(toJSON(found)).to.deepEqual(
toJSON({
id: created.id,
name: 'new name',
description: features.emptyValue,
}),
);
});

@nabdelgadir nabdelgadir force-pushed the refactor-tests branch 2 times, most recently from b81937c to 7a5faa7 Aug 27, 2019

@agnes512 agnes512 force-pushed the refactor-tests branch from 85c290a to bb71b45 Aug 28, 2019

@bajtos
Copy link
Member

left a comment

Great progress!

The new design using create{ModelName}Repo looks much more correct & robust to me. Well done 馃憦

I found few more places where we need to take into account that certain types will be different depending on the connector, please take a look.

@agnes512 agnes512 force-pushed the refactor-tests branch 2 times, most recently from d8c1a48 to b3c20a4 Aug 30, 2019

@nabdelgadir nabdelgadir force-pushed the refactor-tests branch from f5631c8 to 95d6941 Aug 30, 2019

@bajtos
Copy link
Member

left a comment

The patch looks mostly good now, I have few last & hopefully minor comments to address.

@bajtos bajtos dismissed their stale review Sep 2, 2019

All major problems have been addressed by now.

@agnes512 agnes512 force-pushed the refactor-tests branch 3 times, most recently from ba9cb02 to ea20fba Sep 3, 2019

@bajtos
bajtos approved these changes Sep 3, 2019
Copy link
Member

left a comment

Lovely 馃憦

:shipit: 馃殌

refactor(repository-tests): move relation tests to repository-tests p鈥
鈥ckage

Move these tests to repository-tests package so that they would be tested against real databases such as MongoDB and MySQL.
Introduce a new type MixedIdType to CrudFeatures to solve the different usage of id types in different databases.

Co-authored-by: Nora <nora.abdelgadir@ibm.com>

@agnes512 agnes512 force-pushed the refactor-tests branch from ea20fba to f4f97f8 Sep 3, 2019

@agnes512 agnes512 merged commit fa2ecdc into master Sep 3, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 91.632%
Details

@agnes512 agnes512 deleted the refactor-tests branch Sep 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.