Spike: why are relation tests so slow and how to fix that

[bajtos]

See https://github.com/strongloop/loopback-next/issues/5670. Most of the slow tests (taking more than a second to complete) are for the relations.

• lb4 relation generates model relation for existing property name verifies that a preexisting property will be overwritten: 2675ms
• lb4 relation add new controller to existing index file check if the controller exported to index file : 2026ms
• lb4 relation add controller to existing index file only once check if the controller exported to index file only once: 1961ms
• lb4 relation updates property decorator when property already exist in the model: 1566ms
• lb4 relation HasOne rejects relation when source key already exist in the model: 1362ms
• lb4 relation HasOne Execute relation with existing relation name rejects if the relation name already exists in the repository: 1344ms
• lb4 relation HasMany Execute relation with existing relation name rejects if the relation name already exists in the repository: 1325ms
• lb4 relation rejects relation when destination model doesn't have primary Key: 1320ms tsdocs runs api-extractor: 1280ms
• lb4 relation HasMany rejects relation when source key already exist in the model: 1254ms

Let's investigate why are these tests so slow. Do they have expensive setup? Can we find a more efficient way how to test the relation generator?

Acceptance criteria

• [ ] Find out what's taking so long when testing lb4 relation
• [ ] Propose a solution how to improve the speed
  • If it's hard to find a solution in limited time, we can figure out some plans to try.
• [ ] Update https://github.com/strongloop/loopback-next/issues/5670 or open follow-up issues to implement the proposal

[agnes512]

As I commented in https://github.com/strongloop/loopback-next/issues/6131#issuecomment-679297030. The way we check pk and fk in lb4 relation couldn't detect model inheritance.

[agnes512]

Current relation CLI main building relation process:

Prompt:

1. Select relation type
2. Select source/target models[read]
3. Check if corresponding repos exist [read]
4. Check if the source model pk exists [read]
5. Prompt for foreign key
6. Check if the fk exists
7. Prompt for getter/relation name
8. Check if the relation name exists [read]
9. Prompt for inclusion resolver

Scaffold:

1. Check the type of the fk [read]
2. Check if the decorator needs to be changed [read]
3. Update models [write]
4. Update repos [write]
5. Create a controller and update index file [write]

Current design pros and cons.

Read files frequently slows down the process. That's why some tests take a while to run. For example, test case lb4 relation generates model relation for existing property name verifies that a preexisting property will be overwritten: 2675ms creates both hasMany and blongsTo relations, which makes it extremely slow.

PROS:

• Checks the existence of pk/fk/relation name and validates them, so that no invalid (models have no id) or duplicate relations would be created, which the community complained a lot before.

CONS:

• read/write files takes LOTS of time
• The way it checks pks couldn't detect inheritance. See issue: https://github.com/strongloop/loopback-next/issues/6131

Proposal

IMO the relation CLI should keep those checks and validations. lb4 relation is one of the basic commands. We should make sure it fails invalid cases and generate valid relations for users (esp new users). Therefore, it should preserve those validations. For that reason, I propose the following to improve the test speed:

• Refactor test cases and remove redundant ones. For example, the second test case below is redundant:

  context('generates model relation with custom relation name', () => {
    const promptArray = [
      {
        relationType: 'belongsTo',
        sourceModel: 'Order',
        destinationModel: 'Customer',
        foreignKeyName: 'customerId',
        relationName: 'my_customer',
      },
      {
        relationType: 'belongsTo',
        sourceModel: 'OrderClass',
        destinationModel: 'CustomerClass',
        relationName: 'my_customer',
      },
    ];
    ...

• Refactor the code. Try to just read corresponding files once. For example, current design reads the repository files twice (checks the existence of the repo and the input relation name). Moreover, remove some checks. For example, the check for pks on source/destination models can be removed. Let the code in @repository handle it.

• To fix the inheritance issue, prompt for the name and type of the source (like what controller CLI does). This means that it will be user's responsibility to provide valid inputs.

[jannyHou]

1. remove redundant ones

If they don't decrease the coverage, or we have a good reason not testing them, they should definitely be removed.

2. Refactor the code. Try to just read corresponding files once.

👍

[agnes512]

Closing as done, see https://github.com/strongloop/loopback-next/pull/6937