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

(feat): 4246 nnnnat new mocks factory #4276

Merged
merged 26 commits into from
Jun 4, 2018

Conversation

nnnnat
Copy link
Contributor

@nnnnat nnnnat commented May 25, 2018

Resolves #4246
Impact: minor
Type: feature|refactor

Issue

Writing GQL integration test has become difficult because we've been creating mock data ad-hoc.
More info here

Solution

  1. Create a new @reactioncommerce npm package that can take the de-meteorized SimpleSchema and create a mocks factory for each schema. See data-factory
  2. Create a new test-utils/helper that will build a mocks factory using the data-factory package and export a Factory object to be used in the integration test.
  3. Removing some leftover meteor deps from the schemas that we missed in PR 4266.
  4. Replacing Tags integration test mock data with mocks from the test utils Factory.

Breaking changes

  1. Possible breaking change: RC's product variant schema still had a meteor dep inside one of the properties. This is being removed/refactored to not use the ReactionProduct API from lib/api

Testing

  1. Spin up a fresh RC shop confirm it still works.
  2. Create, edit and archive a new Product
  3. Run integration test docker-compose run devserver npm run test:integration

@nnnnat nnnnat changed the base branch from master to release-2.0.0 May 26, 2018 02:58
@nnnnat nnnnat self-assigned this May 31, 2018
@aldeed aldeed changed the base branch from release-2.0.0 to release-1.13.0 June 1, 2018 20:38
@nnnnat nnnnat changed the title [WIP](feat): 4246 nnnnat new mocks factory (feat): 4246 nnnnat new mocks factory Jun 1, 2018
@nnnnat nnnnat requested review from aldeed and removed request for aldeed June 1, 2018 22:55
@nnnnat nnnnat changed the title (feat): 4246 nnnnat new mocks factory [WIP] (feat): 4246 nnnnat new mocks factory Jun 2, 2018
@nnnnat nnnnat changed the title [WIP] (feat): 4246 nnnnat new mocks factory (feat): 4246 nnnnat new mocks factory Jun 4, 2018
@nnnnat nnnnat requested a review from aldeed June 4, 2018 14:54
Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

A few minor inline comments, but everything looks good in testing. Specifically

  • Created and published product. New product has default handle
  • Updated handle. The entered value is properly slugified
  • Made sure I can't submit an empty inventory quantity when inventory tracking is enabled

@@ -0,0 +1,7 @@
import SimpleSchema from "simpl-schema";

SimpleSchema.extendOptions([
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment above this since it might not be obvious why.

// These options are added by the `aldeed:schema-index` Meteor package, but because that
// is a Meteor package and we also load the schemas in a non-Meteor Node app, we need to do it here.

/**
*
* @method hasChildVariant
* @summary Return true if a Product or Variant has at least 1 child Product.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add to the end of summary: "that is visible and not deleted"

mockCollections.Products.findOne.mockReturnValueOnce(Promise.resolve(mockVariants[1]));
const spec = await hasChildVariant(internalVariantIds[0], mockCollections);
expect(spec).toBe(true);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Because you're mocking the findOne, this test isn't really any different from the one above it. I'd just go with the two tests, true and false.

If you wanted to actually test the findOne selector, you'd have to make some integration tests for it. I don't think that's necessary since it's a simple function.

@nnnnat
Copy link
Contributor Author

nnnnat commented Jun 4, 2018

@aldeed I've made the requested changes and it's ready for review.

@aldeed aldeed merged commit 3f05ee4 into release-1.13.0 Jun 4, 2018
@aldeed aldeed deleted the feat-4246-nnnnat-new-mocks-factory branch June 4, 2018 20:28
@nnnnat nnnnat restored the feat-4246-nnnnat-new-mocks-factory branch June 7, 2018 21:32
@aldeed aldeed deleted the feat-4246-nnnnat-new-mocks-factory branch June 8, 2018 09:06
@spencern spencern mentioned this pull request Jun 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants