tests(repository): add missing unit tests

[b-admike]

Fixes #1379. Add unit tests for the following files:

• constraint-utils.ts
• relation-factory.ts
• relation-repository

Checklist

• 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

[bajtos]

Yay for adding more tests 👍

I have few suggestions on how to make the test code easier to understand and maintain, see my comments below.

[bajtos]

When reading this test in isolation, it's not clear where {a:true} is coming from.

If the test depends on specific values passed to constrainFilter, then these specific values need to be provided in the test.

Alternatively, refer to originalFilter properties in the assertion, e.g.

expect(result).to.deepEqual({
  fields: originalFilter.fields,
  limit: originalFilter.limit,
  where: Object.assign({}, originalFilter.where, {id: '5'}),
});

[b-admike]

Agreed :) done.

[bajtos]

Positional arguments are difficult to understand in this case - what is undefined standing for?

Please consider using a single argument containing an object with named properties.

givenAFilter({fields: undefined, where: {id: '10'}, limit: undefined});
// I think this can be further simplified to
givenAFilter({where: {id: '10'}});

Do we actually need to pass that data object through givenAFilter, considering that the input is already a valid filter and most likely the same value as will be returned by givenAFilter?

[b-admike]

I agree that using givenAFilter function is moot for the filters I use in the tests besides originalFilter. I've removed the parameter(s) and defined the filters in their respective test cases.

[bajtos]

We should have only one test that's verifying whether constraintFilter changes or preserves filter properties we are not changing in the test (fields, limit, etc.).

In your current test design, if we add another property to originalFilter, let's say include, we would have to update all tests to check include too.

What to do instead: use one of the contain* matchers to verify only the property this test is interested in.

expect(result).to.containEql({
  where: /* expected where */
});

A full test case as I would write it, taking into my consideration the previous comment too.

const input = givenAFilter({where: {x: 'x'}});
const constraint = givenAWhereConstraint({id: '10'});
const result = constrainFilter(input, constraint);
expect(result).to.containEql({
  where: {x: 'x', id: '10'}
});

[b-admike]

Done :-).

[bajtos]

Empty test cases - please implement the actual test code or remove the test case entirely.

[b-admike]

I'm currently implementing them, but will remove for now 👍

[bajtos]

In the test, it's not clear where is x: 'x' coming from - see my earlier comments.

[bajtos]

In the test, it's not clear where is x: 'x' coming from - see my earlier comments.

[bajtos]

In the test, it's not clear where x: 'x' and y: 'y' are coming from - see my earlier comments.

[bajtos]

ditto

[shimks]

Do we need to assign these variables to empty objects?

[shimks]

I'd appreciate a comment here to let other people know what inputFilter and inputWhere looks like as objects

[bajtos]

The purpose of a test-data builder like givenAFilter is to create any valid filter, this generic value should be used only in places where the test does not really care about what filter is used.

For tests that where the actual filter is important (e.g. because assertions make assumptions about inputs), it's important to capture the important input data directly in the test. See also my earlier comment #1383 (comment) and the example in #1383 (comment) showing how to write a test that's using these test-data builders and yet makes it clear which part of the input filter is important and where the values used in assertions are coming from.

A slightly improved example:

const input = givenAFilterWithWhere({x: 'x'});
const constraint = givenAWhereConstraint({id: '10'});
const result = constrainFilter(input, constraint);
expect(result).to.containEql({
  where: {x: 'x', id: '10'}
});

My point is that if you feel we need a comment to explain what inputFilter and inputWhere looks like to make it easier to understand what the tests are doing, then we should improve the tests instead.

[shimks]

EDIT: solved by importing the file with the factory function as a namespace (sinon.spy(factoryNamespace, 'hasManyRepositoryFactory')). It's weird how sinon doesn't allow you to properly spy a function just by itself. Does anyone know the justification for that?

I'm trying to set up a spy for DefaultHasManyEntityCrudRepository to write the unit tests for hasManyRepositoryFactory function and am having a hard time.

      const spy = sinon.spy(DefaultHasManyEntityCrudRepository);

      hasManyRepositoryFactory(4, meta, orderRepo);

      sinon.assert.calledWithNew(spy);
      sinon.assert.calledWith(spy, orderRepo, {customerId: 4});

This feels like it should work, but it doesn't. I've also tried sinon.spy(DefaultHasManyEntityCrudRepository.prototype, 'constructor') but that one was no good either.
I've tried googling the issue but I haven't been able to find anything useful.

Strangely enough, spy doesn't seem to work when spying a function that's being used in another. For example:

      function factory() {
        return create();
      }

      function create() {
        return {foo: 'bar'};
      }

      const spy = sinon.spy(create);
      const foobar = factory();
      expect(foobar).to.eql({foo: 'bar'});
      sinon.assert.called(spy);

The first assertion will pass, but the second will fail. Is there a gap in my knowledge with sinon as to why this shouldn't work?

@strongloop/sq-lb-apex Do you have any suggestions on what I could do?

[shimks]

Generics are set manually since typescript would crap out if the instance being returned was first assigned to a variable

[shimks]

The code is in a rough state right now, but I wanted to get feedback on whether this single test case is sufficient for the unit test.

[bajtos]

This is modifying global state which I find rather evil. You are not restoring DefaultHasManyEntityCrudRepository after the tests, therefore other tests we may write later will see mocked version instead of the real implementation.

If you want to test relationRepositoryFactory in isolation, then you need to find a way how to inject DefaultHasManyEntityCrudRepository constructor to the factory function. That will allow you to spy on the class locally.

const spy = sinon.spy(DefaultHasManyEntityCrudRepository);
// the spy should be used as follows:
// new spy(4, meta, orderRepo);

// This assumes that RelationFactory accepts a key/value map of relation repository classes
new RelationFactory({hasMany: spy}).createHasMany(4, meta, orderRepo);
// etc.

To be honest, I am not sure how much value there is in such unit tests. Personally, I'd look into ways how to test relationRepositoryFactory in an integration style, where we let the factory create a real repository.

---

I'm trying to set up a spy for DefaultHasManyEntityCrudRepository to write the unit tests for hasManyRepositoryFactory function and am having a hard time.

      const spy = sinon.spy(DefaultHasManyEntityCrudRepository);
hasManyRepositoryFactory(4, meta, orderRepo);

      sinon.assert.calledWithNew(spy);
      sinon.assert.calledWith(spy, orderRepo, {customerId: 4});

Strangely enough, spy doesn't seem to work when spying a function that's being used in another.
The first assertion will pass, but the second will fail. Is there a gap in my knowledge with sinon as to why this shouldn't work?

sinon.spy cannot rewrite function references stored in variables for you, that's a limitation of JavaScript language. Here is an equivalent of what you are trying to do:

let a = 1;
spy(a); // spy is supposed to set "a" to 2
expect(a).to.equal(2);

[shimks]

@bajtos I'm not sure if I agree with your proposal to test this function in integration style anymore. If we were to test the factory function integration style, we'd just end up with what we have for the acceptance test right now, where we assert for products of function calls (create/find) of the constrained repository produced by the factory function. I feel this would be better covered by tests on DefaultHasManyEntityCrudRepository constructor itself.

For the sake of increasing our coverage, I thought testing whether the factory function passes the correct parameters into the relation repository constructor would somewhat fulfill the factory function's requirement to have tests.

What do you think?

Nvm, I think we can just move the acceptance test into integration and call it done :p

My old review is no longer relevant.

[raymondfeng]

What's the rationale behind this? Do we want to allow constraint to be either a Filter or Where?

[b-admike]

Yeah that was the use case I had in mind (if not a filter, then treat it as a where object). Thoughts?

[raymondfeng]

Not sure if the rename is better.

[shimks]

the rename makes sense to me since in the future we're going to be using the same factory function to produce other constrained repositories of different relations

[bajtos]

he rename makes sense to me since in the future we're going to be using the same factory function to produce other constrained repositories of different relations

Let's defer the rename until we actually stat adding support for other relation types please.

I am not sure if it will be possible to have a single factory function, considering that each relation type needs a different repository interface.

[jannyHou]

I would +1 for defer the rename.
The returned value of it is still HasManyEntityCrudRepository<T>, which implies it's a factory for a specific relation

[jannyHou]

Any reason to loose the type to be anyObject? I think we'd better not accept random constraint fields other than the ones in Filter.

[shimks]

feedback applied

[bajtos]

I quickly skimmed through the changes because I cannot tell what's changed since my last review and don't see any obvious problem.

The patch LGTM as far as I am concerned, but please get approval from other commenters before landing.

[bajtos]

if not a filter, then treat it as a where object). Thoughts?

I am little bit worried about the confusion this dual type (Filter | Where) can cause between users - I personally would not be sure which form to use when writing the code and whether a correct form is used when reading code.

Having said that, I don't have any good arguments to support my concerns, and I do see value in allowing both Filter and Where objects. I'll leave the ultimate decision on this to somebody else to make.

[b-admike]

I see your concern @bajtos, but yea it's to provide value so that when users provide a Where object for a constraint, we coerce that to a filter and provide convenience. We will document it in the TSDocs and if it confuses users like you said, we can be remove it later.

[jannyHou]

🎉 LGTM