MB-12189 set up fe fake data generator and factory tools #9142

felipe-lee · 2022-09-01T17:54:27Z

Jira ticket for this change

Summary

This PR sets up our FE to be able to have factories to more easily generate test data.

Document with some findings and thoughts: 🔒 https://dp3.atlassian.net/wiki/spaces/MT/pages/1895071898/Test+factories+and+fake+data+generators

Main changes

Added faker and test-data-bot to js dev deps
Updated eslintrc w/changes needed for factories
- This is what triggered the IS3 review.
- Set up stricter checks for extraneous deps so that we won't just allow any extraneous deps, but still allow dev dependencies where needed.
- Also allow param-reassignment so that factories can more easily work with objects in the postBuild step.
Add faker setup for FE factories
- This sets up faker to use en_US as the locale for generated data
- Also sets up the fake function that will be used by factories to generate fake data.
Add factory for addresses
- This serves as a simple factory to showcase how factories work, and to make it easier to now build out other factories that will need addresses, e.g. service members, shipments, etc.
- It includes setting field attributes per build, postBuild function, traits, and traits with postBuild.
Update a set of tests to use addressFactory to showcase using factories

Other changes

Add @storybook/addon-actions as an explicit dev dependency
- This was needed because of the eslint config changes I made. We already have the package because of other dependencies, but since we import it in some files, it's better to list it as an explicit dependency.

Testing the changes

💻

Running Tests

yarn test src/pages/MyMove/Profile/ResidentialAddress.test.jsx

Verification Steps for Author

These are to be checked by the author.

Request review from a member of a different team.
Have the Jira acceptance criteria been met for this change?

We import it in a few places and with the eslint config I'm going to commit next, we need to have it in our dependencies

Set up stricter checks for extraneous deps so that we won't just allow any extraneous deps, but still allow dev dependencies where needed. Also allow param-reassignment so that factories can more easily work with objects in the postBuild step.

robot-mymove · 2022-09-01T18:05:11Z

	Messages
📖	🔗 MB-12189

Generated by 🚫 dangerJS against 2330244

…nerator-and-factory-tools

The `zipCodeByState` function uses ranges of numbers for each state to generate the zip, but since some state zip codes start with 0's, they get stripped out. This adds the zero back to the front if needed.

felipe-lee · 2022-09-01T23:39:01Z

.eslintrc.js

@@ -69,7 +83,6 @@ module.exports = {
    {
      files: ['*.stories.js', '*.stories.jsx', 'setupTests.js'],
      rules: {
-        'import/no-extraneous-dependencies': 'off',


I pulled this out of here and put it above so that we only allow dev dependencies but still check for other extraneous dependencies.

felipe-lee · 2022-09-02T00:03:26Z

src/utils/test/factories/base.js

+const fake = (callback) => {
+  return perBuild(() => callback(faker));
+};


This is partially syntactic sugar. It makes it so than in a factory, instead of having to write:

... fields: { streetAddress1: perBuild(() => faker.address.streetAddress()) } ...

you can instead write:

... fields: { streetAddress1: fake((f) => f.address.streetAddress()), } ...

which, tbh, imo isn't a huge win for this alone.

But, one other advantage is that it lets us set the locale once and then everything that uses fake will automatically get it. In version 3.0.0 of faker, there will be per localization packages, which will make this a little less necessary since we could just make sure we always import from the same package everywhere. Though it might still be beneficial to have it here so that we don't have to be ensuring we import from the same place and just leave that here.

That seems worth it

pearl-truss · 2022-09-02T13:29:00Z

src/utils/test/factories/address.js

+export const BLANK_ADDRESS = 'blank';
+export const ADDRESS_WITHOUT_COUNTRY = 'omitCountry';
+
+const addressFactory = build({


Question: Will engineers still be able to add specific values for certain fields? For instance with pricing wanting to choose zipcodes that are a certain distance from each other

Yeah I think we should be able to do that. If you want to just pass some override values you can do that by doing something like:

const fakeAddress = addressFactory({ overrides: { streetAddress1: '1600 Washington Ave' } });

For something like the postalCode that is set as part of the postBuild, I don't think there is a way to pass in a value and set the value. Though we can just create a value using the factory and then do a manual override by doing something like:

const fakeAddress = addressFactory({ overrides: { streetAddress1: '1600 Washington Ave' } }); fakeAddress.postalCode = '90210';

Do we think it's worth turning the override keys into constants? I'm working on the ServiceWorker factory, and the keys down in the weight_allotment can be tricky to get correct in an override. Constantizing them would at least make them available to Intellisense.

Scott and I met earlier and I learned from him that in order for an override to work, it needs to be defined as one of the base fields. So if I add postalCode to the field definitions, we can pass in an override. The postBuild can still overwrite it, but I can add a conditional to leave it as is if it's already been set. I can push up a commit with those changes.

We also talked about having constants for the fields. I agree it would be good to do that because intellisense is nice and the point of these factories is to speed up test writing. We talked about a few options for how that could work. I'm going to push up one version, but plan on talking more about it during our PR review time on Tuesday, and bringing things up at the BE/FE checkin.

pearl-truss · 2022-09-02T13:29:25Z

src/utils/test/factories/base.js

+const fake = (callback) => {
+  return perBuild(() => callback(faker));
+};


That seems worth it

pearl-truss · 2022-09-02T13:31:14Z

Request: can a link to this PR be added to the ADR for this concept?

ronaktruss · 2022-09-02T13:33:29Z

src/utils/test/factories/address.js

+  },
+});
+
+export default addressFactory;


What's the rationale behind having this be a default import over a named import?

Ah, that was more because I started with only one export and our eslint config requires it to be default. Then I added more things, but by then I wasn't sure if it was being used in other branches so I didn't want to cause merge issues. I'm happy to change it though and just communicate out on the channel in case anyone has a branch and is using this.

Ryan-Koch

Giving a 👍 for the eslintrc change

felipe-lee · 2022-09-02T19:35:55Z

Request: can a link to this PR be added to the ADR for this concept?

Yeah, great idea! I can open up a PR for the ADR.

…nerator-and-factory-tools

felipe-lee · 2022-09-02T22:46:24Z

Request: can a link to this PR be added to the ADR for this concept?

Yeah, great idea! I can open up a PR for the ADR.

@pearl-truss here's the other PR: transcom/mymove-docs#205

src/utils/test/factories/address.js

…nerator-and-factory-tools

felipe-lee · 2022-09-09T01:17:52Z

src/utils/test/factories/base.js

+  return perBuild(() => callback(faker));
+};
+
+const loadSpec = (fileName) => {


Got the idea for this from swagger-api/swagger-js#1410

Came up while I was looking for a way to use the swagger client with local files. I didn't want to point to a URL since the swagger files could change with a PR and then we'd need to update the path for those. An alternative is linked to in that thread to spin up a local server just to serve the file, but that seems like more work than is needed. Especially because once the yaml is parsed and converted to json, we don't really need the swagger client, we can access all of our definitions through the json this outputs.

So I tried the codegen tool a bit running:

docker run --rm -v ${PWD}:/local swaggerapi/swagger-codegen-cli generate \ -i local/swagger/internal.yaml \ -l javascript \ -o /local/src/utils/test/swagger-client/internal \ -DmodelDocs=false -DmodelTests=false -DapiDocs=false -DapiTests=false

It then needs to be installed with:

yarn add --dev file:./src/utils/test/swagger-client/internal

This seems to give us access to the definitions. One nice thing about this is that it gives us code completion since there are actual variables for properties like the state enum, rather than just working with json. I haven’t gotten a chance to mess with it too much though.

duncan-truss · 2022-09-09T16:41:57Z

src/utils/test/factories/address.js

+    [ADDRESS_FIELDS.COUNTRY]: 'US', // Likely change once we support more than just CONUS moves.
+  },
+  traits: {
+    [ADDRESS_TRAITS.BLANK]: {


This isn't really a factory question, is there a reason we would have an address with blank fields? I see that streetAddress2 gets saved as a blank string in the customer app. Or is there some null/undefined behavior where this is useful?

It looks like we have to define all of the fields on the model and then make a trait if we ever want to remove fields down to say what is only required?

Ah, that came up because when I initially started using this in the residential address tests, there had been an empty address. Though after looking at the internal yaml, it looked like the address as a whole was nullable, and could be omitted when empty, so I ended up not using this. I should probably just remove the trait since there isn't a current use case for it.

To your point about having to define all the fields and then removing them, that is what needs to be done because if you don't define a field, you can't override it at build-time, which is a bit annoying. Thanks for reminding me, I'll add that to the things that would make it easier to work with factories

duncan-truss · 2022-09-09T16:43:40Z

src/utils/test/factories/address.js

+};
+
+export const addressFactory = build({
+  fields: {


Should id be included as an option here to represent an already created record?

Ah, yeah I need to add that one and eTag. I can push that up today.

duncan-truss · 2022-09-09T16:46:30Z

src/utils/test/factories/address.js

+
+        return address;
+      },
+    },


It could be a good pattern to start where there is a 'required' trait that doesn't automatically fill in all of the optional fields. This may not be perfect across forms but usually we are worried about validations in our tests.

Good point, that would probably be a good thing to have, I can add that.

duncan-truss

Seems like a good start to me. I am curious how it will work with nested objects but this was a good first demonstration case.

…nerator-and-factory-tools

Update `NO_COUNTRY` trait to be `ONLY_BASIC_ADDRESS` because the new fields also need to be removed for cases when country was removed, at least in the tests it's being used in currently.

felipe-lee · 2022-09-09T23:26:14Z

src/utils/test/factories/base.js

+const loadSpec = (fileName) => {
+  const yamlPath = path.join(process.cwd(), `./swagger/${fileName}`);
+
+  // RA Summary: eslint - security/detect-non-literal-fs-filename - OWASP Path Traversal


https://transcom.github.io/mymove-docs/docs/backend/guides/guide-to-static-analysis-security-workflow/

NamibiaTorres

Nice job so far! I was just learning about factories in React so it was really interesting seeing how they are implemented!

felipe-lee · 2022-09-26T19:56:03Z

Closing in favor of #9233

felipe-lee added 5 commits September 1, 2022 01:06

MB-12189 Add faker and test-data-bot to js dev deps

d87606a

MB-12189 Add @storybook/addon-actions as an explicit dev dependency

240d91b

We import it in a few places and with the eslint config I'm going to commit next, we need to have it in our dependencies

MB-12189 Add faker setup for FE factories

c2590b4

MB-12189 Add factory for addresses

6055ab6

felipe-lee added the A-Team label Sep 1, 2022

felipe-lee self-assigned this Sep 1, 2022

felipe-lee requested a review from scottmries September 1, 2022 17:54

felipe-lee added 4 commits September 1, 2022 18:38

Merge branch 'master' into felipe-lee/MB-12189-set-up-fe-fake-data-ge…

adbe43b

…nerator-and-factory-tools

MB-12189 Add 0 pad to postal codes for states w/leading zeros

a7dca5f

The `zipCodeByState` function uses ranges of numbers for each state to generate the zip, but since some state zip codes start with 0's, they get stripped out. This adds the zero back to the front if needed.

MB-12189 Add traits to addressFactory

dd3e109

MB-12189 Update a set of tests to use addressFactory

ca6f336

felipe-lee marked this pull request as ready for review September 1, 2022 23:19

felipe-lee requested review from a team, NamibiaTorres, duncan-truss, jenniferlynparsons, pearl-truss, ronaktruss and bellex3 September 1, 2022 23:19

felipe-lee commented Sep 1, 2022

View reviewed changes

MB-12189 Remove commented out file globs

51456d8

felipe-lee commented Sep 2, 2022

View reviewed changes

pearl-truss reviewed Sep 2, 2022

View reviewed changes

ronaktruss reviewed Sep 2, 2022

View reviewed changes

Ryan-Koch approved these changes Sep 2, 2022

View reviewed changes

Merge branch 'master' into felipe-lee/MB-12189-set-up-fe-fake-data-ge…

bd3586e

…nerator-and-factory-tools

felipe-lee mentioned this pull request Sep 2, 2022

MB-12189 Add links to poc and implementation PRs for ADR 68 transcom/mymove-docs#205

Merged

felipe-lee added 3 commits September 2, 2022 23:33

MB-12189 Replace address factory field names with constants

2f952b5

MB-12189 Make it possible to override postal code in the addressFactory

4eefd70

MB-12189 Update addressFactory to not be the default export

8882b14

felipe-lee commented Sep 2, 2022

View reviewed changes

src/utils/test/factories/address.js Outdated Show resolved Hide resolved

felipe-lee added 5 commits September 8, 2022 18:04

Merge branch 'master' into felipe-lee/MB-12189-set-up-fe-fake-data-ge…

b600b8a

…nerator-and-factory-tools

MB-12189 Add js-yaml to js dev dependencies

afa5d70

MB-12189 Add way to work with api specs in factories

81c7f21

MB-12189 Update address factory to use internal spec for state options

45702d2

MB-12189 Update address traits to be grouped

482681a

felipe-lee commented Sep 9, 2022

View reviewed changes

duncan-truss reviewed Sep 9, 2022

View reviewed changes

duncan-truss approved these changes Sep 9, 2022

View reviewed changes

felipe-lee added 4 commits September 9, 2022 20:24

Merge branch 'master' into felipe-lee/MB-12189-set-up-fe-fake-data-ge…

74beb9e

…nerator-and-factory-tools

MB-12189 Remove blank address trait since we don't need it

8a37ad1

MB-12189 Add id and eTag to the address factory fields

adf71ba

Update `NO_COUNTRY` trait to be `ONLY_BASIC_ADDRESS` because the new fields also need to be removed for cases when country was removed, at least in the tests it's being used in currently.

MB-12189 Add trait for only having required fields for address

2330244

felipe-lee commented Sep 9, 2022

View reviewed changes

NamibiaTorres reviewed Sep 11, 2022

View reviewed changes

felipe-lee closed this Sep 26, 2022

felipe-lee deleted the felipe-lee/MB-12189-set-up-fe-fake-data-generator-and-factory-tools branch October 11, 2022 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MB-12189 set up fe fake data generator and factory tools #9142

MB-12189 set up fe fake data generator and factory tools #9142

felipe-lee commented Sep 1, 2022 •

edited

Loading

robot-mymove commented Sep 1, 2022 •

edited

Loading

felipe-lee Sep 1, 2022

felipe-lee Sep 2, 2022 •

edited

Loading

pearl-truss Sep 2, 2022

pearl-truss Sep 2, 2022

ronaktruss Sep 2, 2022

scottmries Sep 2, 2022

felipe-lee Sep 2, 2022

felipe-lee Sep 2, 2022

pearl-truss Sep 2, 2022

pearl-truss commented Sep 2, 2022

ronaktruss Sep 2, 2022

felipe-lee Sep 2, 2022

Ryan-Koch left a comment •

edited

Loading

felipe-lee commented Sep 2, 2022

felipe-lee commented Sep 2, 2022

felipe-lee Sep 9, 2022

felipe-lee Sep 9, 2022

duncan-truss Sep 9, 2022

felipe-lee Sep 9, 2022

duncan-truss Sep 9, 2022

felipe-lee Sep 9, 2022

duncan-truss Sep 9, 2022

felipe-lee Sep 9, 2022

duncan-truss left a comment

felipe-lee Sep 9, 2022

NamibiaTorres left a comment

felipe-lee commented Sep 26, 2022

MB-12189 set up fe fake data generator and factory tools #9142

MB-12189 set up fe fake data generator and factory tools #9142

Conversation

felipe-lee commented Sep 1, 2022 • edited Loading

Jira ticket for this change

Summary

Testing the changes

Running Tests

Verification Steps for Author

robot-mymove commented Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

felipe-lee Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pearl-truss commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryan-Koch left a comment • edited Loading

Choose a reason for hiding this comment

felipe-lee commented Sep 2, 2022

felipe-lee commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncan-truss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NamibiaTorres left a comment

Choose a reason for hiding this comment

felipe-lee commented Sep 26, 2022

felipe-lee commented Sep 1, 2022 •

edited

Loading

robot-mymove commented Sep 1, 2022 •

edited

Loading

felipe-lee Sep 2, 2022 •

edited

Loading

Ryan-Koch left a comment •

edited

Loading