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

Document how to write factory functions with value overwrites #1508

Closed
KevinMind opened this issue Nov 1, 2022 · 10 comments · Fixed by #3207
Closed

Document how to write factory functions with value overwrites #1508

KevinMind opened this issue Nov 1, 2022 · 10 comments · Fixed by #3207
Assignees
Labels
c: docs Improvements or additions to documentation p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@KevinMind
Copy link
Contributor

Clear and concise description of the problem

Most who use faker end up wrapping faker methods into "factory functions" that compose those methods into usable objects that represent their underlying data needs.

Example in faker docs itself: https://fakerjs.dev/guide/usage.html#create-complex-objects

import { faker } from '@faker-js/faker';

class User { ... }

function createRandomUser(): User {
  return {
    _id: faker.datatype.uuid(),
    avatar: faker.image.avatar(),
    birthday: faker.date.birthdate(),
    email: faker.internet.email(),
    firstName: faker.name.firstName(),
    lastName: faker.name.lastName(),
    sex: faker.name.sexType(),
    subscriptionTier: faker.helpers.arrayElement(['free', 'basic', 'business']),
  };
}

const user = createRandomUser();

There are several concepts in this pattern that could be extracted into a relatively lightweight method that could ship along with faker giving first class support and a strong foundation for this use case being implemented "correctly" (I'll lay out my argument for why there should be a correct way to do this"

Suggested solution

We implemented a version of this in our team internally and would like to open source it. Currently there is no option to opens source within our company, so I wanted to propose we add it directly into faker. The method is implemented and tested already and the actual code is very short so it would likely be a low maintenance addition.

Below I've added our README we use to document the method. The actual method is just a few lines and is just a simple deep merge. It is currently implemented using deepmerge but could be refactored to be without dependencies.

Most of the magic is in the custom types. These types enforce good best practices when creating complex objects by leveraging typescript to enforce only required properties to be defined by default and giving an optional override argument to the factory function to allow for defining optional properties.

<----- Here is the README for our implementation ----->


title: Writing Tests with Mock Factories

Getting started

When you want to create a new mock, create a fixtures folder in the component directory and
place an index.ts in it. Mocks can be shared between unit tests and stories.

Folder structure

 📂Component
 ┣ 📂tests
 ┣ 📂fixtures
 ┃ ┗ index.ts
 ┣ Component.tsx
 ┣ index.ts

Then create the mock by passing required properties:

// component/fixtures/index.ts
import {ComponentProps} from 'react';
import {createMockFactory} from 'tests/utilities';

import Component from '../component';

type Props = ComponentProps<typeof Component>;

export const mockComponentProps = createMockFactory<Props>(() => {
  return {
    property: 'some default string',
    otherProperty: faker.random.number(),
  };
});

Finally, use the mock into your test file or stories:

// component/tests/component.test.tsx or
// component/tests/component.stories.tsx
import {mockComponentProps} from '../fixtures';

// Get values from mock
const defaultProps = mockComponentProps();

// Or use customize values
const customProps = mockComponentProps({
  property: 'some custom string',
});

Important: do not export the mock factories through the component. Doing so might cause the
staging and production builds to fail, as the /tests folder is not resolved in those builds.

Conventions:

  • name mock factories with mock${InterfaceName} so an interface with name FooBar would have a
    mock factory named mockFooBar
  • name component mock factories with suffix Props so component with name Foo would have a
    mock factory named mockFooProps
  • name the folder where mocks are exported fixtures and export named mock factories from an
    index.ts file in this folder.
  • mock factories that should be considered public should be imported from the component fixtures
    components/MyComponent/fixtures index.ts file.

defaults argument

Use createMockFactory<T> to generate reusable, composable, and strongly typed mock data objects
in your stories or unit tests. To create a mock factory, just pass a typescript interface and
your defaults data to the createMockFactory method. This method will type check your data,
to make sure it correctly implements all and only all required properties of the interface.

The defaults argument will accept either a static object, or a function that will be called
each time a mock is created, allowing for randomized or dynamic data. The default will also restrict
your default values to required props only, so in the example below, optional must be set to
undefined in the "default" data set. This ensures that the base set of data for an object covers all
and only all of the required properties, making the "default" case more accurately reflect how
typescript will handle your interface.

interface Props {
  disabled: boolean;
  optional?: string;
  nested: MoneyObject;
}

const mockProps = mockMyInterface<Props>({
  disabled: false,
  nested: {
    amount: 10,
    currencyCode: 'USD',
  },
});

overrideValues argument

Continuing with the example from above, call mockProps() to generate mock objects from the
factory.

mockProps() is a generic function, so it also accepts a type ReturnType, the default value
for ReturnType is, well T.

What does that mean? It means that if you call mockProps() with no generic argument, it assumes
you want the function to return a value of the interface the mock is mocking. You can also pass
in any interface that extends T.

That means you could pass in DeepOmitOptional<T> and this would mean mockProps() would only be
allowed to return required properties. you could also pass in T & SuperT where SuperT adds
additional properties.

Here is an example:

interface Person {
  name: string;
  age: number;
  isActive?: boolean;
}

const mockPerson = createMockFactory<Person>(() => ({
  name: faker.name.firstName(),
  age: faker.random.number({min: 0, max: 100}),
}));

interface PersonWithLastName extends Person {
  lastName: string;
}

// mockPerson() only cares that you fulfil the base interface Person.
// so PersonWithLastName can reuse the mock factory of Person, with complete type safety.
mockPerson<PersonWithLastName>({
  ...mockPerson(),
  lastName: 'wow',
});

// required person this will throw an error because isActive is not required on type Person
// type-error isActive of type boolean is not assignable to type undefined
mockPerson<DeepOmitOptional<Person>>({
  isActive: true,
});

The overrideValues argument that is a DeepPartial of your interface.

interface Props {
  string: string;
  optional?: number;
  nested: {
    optional?: boolean;
  };
}
// Good
const part: DeepPartial<Props> = {nested: {optional: false}};

Using a partial of your interface allows each mock instance to override specific properties
and merge them with the values from defaults. A deep partial allows the properties to
be overwritten at any level in the data structure.

interface Props {
  string: string;
  optional?: number;
  nested: {
    optional?: boolean;
  };
}

const bad: DeepPartial<DeepOmitOptional<Props>> = {nested: {optional: false}};
// Bad - optional is optional, so must be undefined

const good: DeepPartial<DeepOmitOptional<Props>> = {
  string: 'banana',
  nested: {optional: undefined},
};
// Good - optional can be initialized as an undefined value.

const alsoGood: DeepPartial<DeepOmitOptional<Props>> = {
  string: 'banana',
  nested: {},
};
// Good - optional can be omitted from nested entirely.

Typescript will throw an error that the property optional incorrectly implements the type
expected, because types boolean and undefined are incompatible.

NOTE:

Your IDE will make creating mock objects really easy if you have the typescript language service enabled. This will allow for visual feedback in your editor, as well as autocomplete of your data objects.

Composing mocks

You can compose mock factories into larger mock factories. This can reduce code duplication
and make your data much more consistent and reliable. Let's say you define a mockFactory for
MoneyObject:

const mockMoneyObject = createMockFactory<MoneyObject>(() => ({
  amount: randomMoneyAmount(),
  currency: 'USD',
}));

You could then use mockMoneyObject(); in the mockProps factory default argument.

const mockProps = createMockFactory<Props>({
  disabled: false,
  nested: mockMoneyObject(),
});

Now each time you create a mock from mockProps, the nested property will use mockMoneyObject to
define its data. Because nested expects the type DeepOmitOptional<MockMoneyObject> our call to
mockMoneyObject() must return only required data.

Instead of having to continuously redefine values for nested interfaces, you can define just a
single mock factory for a typescript interface, and share the mocking logic into any number of use
cases that depend on that interface. Notice though, that if you don't explicitly use
DeepOmitOptional<T>() in these composition cases, the value being passed to a parent "defaults"
argument, will throw an error on the property, instead of the value.

import {createMockFactory} from './createMockFactory';

interface Address {
  street: string;
  zipCode?: number;
}

const mockAddress = createMockFactory<Address>({
  street: faker.address.street(),
});

interface Person {
  name: string;
  address: Address;
}

// in this case the error will focus on address because it returns the incorrect data set,
// but it won't tell you that zipCode is the issue in a clear precise manner.
createMockFactory<Person>({
  name: faker.random.name(),
  address: mockAddress({
    zipCode: 12345,
  }),
});

// in this case the error will drill down to reveal the zipCode value should be undefined.
createMockFactory<Person>({
  name: faker.random.name(),
  address: mockAddress<DeepOmitOptional<Address>>({
    zipCode: 12345,
  }),
});

Compare to alternatives

You might ask, why use a fancy factory method to create some mock objects? Why not just use
static values? Let's compare two code snippets. One using a more basic, 'on-the-fly' getter
function, versus a mock factory.

  1. 'on-the-fly' mock getter
function getMockData(input?: Partial<Props>): Props {
  return merge(
    {
      disabled: false,
      nested: {
        amount: 10,
        currencyCode: 'USD',
      },
    },
    input,
  );
}
  1. mock factory
const mockData = createMockFactory<Props>(() => ({
  disabled: faker.random.boolean(),
  nested: mockMoneyObject(),
}));

The key differences are that in the second case you rely on less boilerplate code, you achieve a
much higher type safety, and you have much more flexibility in how you design your mocks. Default
values exclude optional properties, giving you very reliable mock data. Using a factory method for
generating the mocks themselves makes the solution scalable and easy to adopt.
Allowing static and dynamic objects retains the option of simple static mocks, but further increases
the reliability of the mock with dynamic values, since you don't have to rely on a single value,
but a range of values that match the type criterion.

Alternative

I could open source this under a separate repo, but it just feels so close to faker it made sense to at least open the discussion if this would be something that could belong in faker.

Additional context

No response

@KevinMind KevinMind added the s: pending triage Pending Triage label Nov 1, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Nov 1, 2022

Thanks for sharing a real world usage scenario with us, this really helps us to focus on and provide features that you are using or missing during your work every day.

To sum up your feature proposal in pseudo code:

mock_T_Factory<T> = (overwrite: T) => deepMerge( sample_T_Generator(), overwrite);

IMO this generally a good idea, however I would like to probe into a few design decisions that you have made and that could deviate from other developers expectations.

  • Why do you omit optional properties in the sample factory? IMO you could use faker.helpers.maybe to set or omit it randomly.
  • Why do you always deep merge? I assume some objects are based on "fixed catalogs"/typed variants, that should be replaced as is instead of partially overwriting some properties

@ST-DDT ST-DDT added the s: awaiting more info Additional information are requested label Nov 1, 2022
@KevinMind
Copy link
Contributor Author

KevinMind commented Nov 2, 2022

I can share the code with you once I checkout it is okay internally to share. Just a formality I suppose. But to respond to the probe, your pseudo code is very close to accurate, there are a few tweaks:

  • I've added a merge combinator function to the internal merge call that will replace default arrays with overridden arrays. The caller should decide what the value is instead of merging. We could in essence expose this combinator to the factory itself allowing users to use their own logic. There would be some tradeoffs there but might make it more flexible. There may be other use cases for this like with explicit undefined/null values etc.
  • The internal factory function returns a type ReturnType extends T allowing a caller to extend the factory to return a type that enhances the original. I included a section on that in the previous comment.
  • The types in the actual implementation are a bit more sophisticated to cover all the edge cases.

Regarding omitting optional properties, yeah this is likely the most controversial part of the code but in our experience it helps make mock factories predictable and level. If I create a mock factory mockMyType that is mocking a deep object with many optional/required properties then I can guarantee that any call to mockMyType() without an override argument will return exactly the minimal set of data specified by that type.

Example:

interface Address {
  address1: sting;
  address2?: string;
}

interface Person {
   name: string;
  age?: number;
  address?: Address;
  phone?: string;
}

const mockAddress = createMockFactory<Address>({
  address1: faker.address.streetAddress(),
});

const mockPerson = createMockFactory<Person>({
  name: faker.name.fullName(),
});

With the current proposed implementation, every instance of mockAddress will contain exactly

{
   address1: string;
   address2: never;
}

and every instance of mockPerson will contain exactly

{
  name: string;
  age: never;
  address: never;
  phone: never;
}

I can make explicit assumptions about 100% of calls to the factory functions now. If I were to always randomly decide which optional props existed or didn't if I wanted to make a test case that verified if a given optional property is undefined, I would have to explicitly define that. This makes the override less useful because instead of dealing only with DeepOptional<T> you have to cover T props.

expect(mockPerson({age: undefined}).age).toStrictEqual(undefined); // works every time but is a lot more properties I have to define
expect(mockPerson().age).toStrictEqual(undefined); // maybe true maybe not if we randomly define it on each call

This problem becomes more and more difficult the deeper nested object you get

expect(mockPerson().address.address2).toStrictEqual(undefined); // I have 2 probabilities to deal with now

In practice using a minimalist rule for default values results in test cases that are very easy to understand because all of the instances where you deviate from the default set of data (shape of the object) are explicit.

it('tests with an address', () => {
  const myAddress2 = faker.address.streetSuffix();

  expect(mockPerson({
    address: {
      address2: myAddress2
    }
  }).address.address2).toStrictEqual(myAddress2);
})

I could see a more flexible version of this approach to instead allow some options argument to the factory function that would set an options.randomness or something like that. It could default to zero to maintain the standard behaviour but then if you set randomness to 0.7 then you could use that to randomly specify the optional props.

function createMockFactory<T>(defaults: T, {randomness: number = 0}) {
   return function(overrides: DeepOptional<T>) {
     return map_values_with_randomness(deepMerge(defaults, overrides));
  }
}

We discussed these topics while developing the function and decided that the simpler architecture was better in that it doesn't block you from doing anything, you can always specify whatever you want in the override. But like I said, if there is significant push back from a larger community of devs that have different preferences I think we can adjust to accommodate that.

What do you think would be the best next steps for something like this? Should I open a PR with a sample implementation? Or what would you suggest?

@ST-DDT
Copy link
Member

ST-DDT commented Nov 2, 2022

We will discuss this in our team meeting tomorrow.

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision and removed s: pending triage Pending Triage s: awaiting more info Additional information are requested labels Nov 2, 2022
@ST-DDT ST-DDT removed this from Faker Roadmap Nov 2, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Nov 3, 2022

Team decision

Due to the different expectations regarding the method, we won't add it to our repository directly.
However, we think this might be a good addition for our creating complex object docs.
Before we start adding new content to that section, we have to move that section to its own page first (separate PR).
#1491 would benefit from this as well.

@ST-DDT ST-DDT added c: docs Improvements or additions to documentation s: accepted Accepted feature / Confirmed bug and removed c: feature Request for new feature s: needs decision Needs team/maintainer decision labels Nov 3, 2022
@ST-DDT ST-DDT moved this to Todo in Faker Roadmap Nov 3, 2022
@ST-DDT ST-DDT changed the title Native faker factory function to create composable custom data objects Document how to write factory functions with value overwrites Nov 3, 2022
@KevinMind
Copy link
Contributor Author

Would you be able to elaborate on Due to the different expectations regarding the method.

I would like to use this feedback in continuing development. I will go ahead and open source it independently, maybe there could be a future where the docs link to it? ;)

@ST-DDT
Copy link
Member

ST-DDT commented Nov 4, 2022

Sure, no problem. Please let us know, if you disagree with any of these points.

  • In our perspective that method/feature will most likely not contain any faker method in its implementation. Sure it is useful in combination with faker, but so is deepMerge. In our opinion this is more of a special variant of deepMerge than it is of faker.
  • The implementation requires either the deepMerge dependency or would recreate their logic.
  • We have good reasons for both having and omitting optional properties. We would likely go for to just <T> so that the user can define whether they want them using <OnlyRequired<T>> or not.
  • Also we thought about the nested merge, which is arguably the most useful part of this feature request. The merge strategy is usecase dependent and thus would require a parameter to configure it. However the parameter would most likely contain the actual implementation of the merge, that would otherwise just be in the method itself. And once again there is nothing faker/seed related in the implementation
  • In our little experiments the code wasn't actually shorter than before. (I know that some of that is formatting, but no noticeable gains either)

Here a few examples, mockX => this feature, createX => without this feature

Usage Example

const user = mockUser({ 
    name:  'foobar',
    organization: {
        id: 15,
    }
});
const user = createUser();
user.name = 'foobar';
user.organization.id = 15;

Implementation Example

export const mockUser = createMockFactory<User>(() => {
   name: faker.internet.userName(),
   organization: mockOrganization();
}, optionalMergeStrategyHere); // <-- Not sure how to implement this/how readable the implementation will be

const optionalMergeStrategyHere = (path, key, valueOld, valueNew) => path === '.' && key === 'organization' ? 'DEEP_MERGE' : valueNew;
// Overrides Strategy: Merge
export function createUser(overrides: DeepPartial<User>) {
    return {
           name: faker.internet.userName(),
           ...overrides,
           organization: createOrganization(overrides.organization);
    };
}
// Overrides Strategy: Replace
export function createUser(overrides: DeepPartial<User>) {
    return {
           name: faker.internet.userName(),
           organization: overrides.organization ?? createOrganization();
    };
}

Options Example

export const mockUser = createMockFactory<User>(() => {
   name: faker.internet.userName(),
});

export const mockUserWithOrganization = createMockFactory<User>(() => {
   name: faker.internet.userName(),
   organization: mockOrganization();
});
export function createUser(overrides: DeepPartial<User>, options: { includeOrganization?: boolean } = {}) {
    const { includeOrganization } = options;
    return {
           name: faker.internet.userName(),
           ...overrides,
           organization: faker.datatype.boolean(includeOrganization) ? createOrganization(overrides.organization) : undefined;
    };
}

@KevinMind
Copy link
Contributor Author

Are these points up for rebuttal or is this discussion closed RE including in faker? I think there some advantages missed that I can write up when I have some time.

@ST-DDT
Copy link
Member

ST-DDT commented Nov 5, 2022

Are these points up for rebuttal or is this discussion closed RE including in faker? I think there some advantages missed that I can write up when I have some time.

We are always interested in new points of view and suggestions from our contributors and users.

Could you please explain what the RE in the following sentence means/refers to?

or is this discussion closed RE including in faker?

@KevinMind
Copy link
Contributor Author

RE is like in an email, as in is this discussion closed in response to or regarding including in faker.

All the code here is psuedo code. I just tried to give as much as needed to understand the use case and API. I've gotten a green light for opensourcing the code so far so can put it in a repo and share soon. That being said, here we go :) Sorry if it is long, there is just a lot to unpack.

Comparing create versus mock approaches to creating mocks

In terms of comparing the usage example, if I understand right the createUser approach is like the current docs where you have a function that creates a full T object with no arguments, and then you just set properties on that object after the fact.

function createUser(): User {
  // implement user
}

It's fair to say you might have a preference for setting values via createUser over, specifying values via mockUser. That's an opinion about how you want to define data on an object, which is fair.

However, if some users prefer setting values and others prefer specifying then mockUser is the better choice as it supports both APIs, whereas createUser only allows you to specify properties after the fact. If we tried to implement a concept of overrides into createUser (like you have in some of your examples) we would just end up implementing something very similar to what i'm already proposing.

We can see this in a more complex example: (almost all of the issues in this space arise when you have deep nesting)

interface Phone {
   areaCode: AreaCodeEnum;
   number: number;
}

interface Person {
  name: string;
  phone: Phone;
  age: number;
}

interface Group {
  id: string;
 members: Person[];
}

using mockGroup you can go both ways

// works with specifying directly in the overrides
const mock = mockGroup({
  members: [
    mockPerson({
      phone: {
        areaCode: AreaCodes.Texas
      }
    })
  ]
});

// also works by setting the value after creation
const otherMock = mockGroup();
otherMock.members[0].phone.areaCode = AreaCodes.Texas;

Why restrict optional values at all?

Looking at this example above, you can see why you might want to restrict defining values conditionally in the default function. Let's imagine if members were optional:

interface Group {
  members?: Person[];
}

In the mock

const mock = mockGroup();

mock.members[0]; // this might error if members is not defined. but it might not error if it is defined. I don't know if it is or not

by restricting optional properties we can be 100% sure what the shape of a final mock object will be just by looking at what values are required and what you pass into the overrides

const undefinedMembers = mockGroup(); // I know that members is undefined

const withMembers = mockGroup({members: [...members]}); // I know there are members because they are explicitly defined

// notice group.id is available in both because it is not optional
undefinedMembers.id // works fine
withMembers.id // also works

That said, I think you could implement a probability option in the factory that could allow users to specify the kind of behaviour they want per mock. I demonstrate what that could look like below.

Does this function have anything to do with faker inherently?

Moving onto another point about how close this function is/could be to faker or deepmerge.

... not contain any faker method in its implementation.

  1. If we included the logic for conditionally providing optional values per your feedback you could use faker helpers for that.
  2. I like the idea of injecting faker into the default value function or even improving DX for defining some values.

For point #1

// for now we use `T` but we could allow this feature to be configurable with an option {probability: 0-1} if > 0 then `T` if 0 then `DeepOmitOptional<T>`
function createMock<T>(defaults: T) {
   // shallow loop but would need to be built in deep
   for (let prop in defaults) {
      // pseudo code, would need to figure out how to conditionally delete prop based on probability
      faker.helpers.maybe(defaults[prop], { probability: 0.9 })
   }
}

For #2 We could inject faker by wrapping the factory function (actually making it a mock factory)

function createMockFactory(options) {
  return function factory<T>(defaults: Thunk<T>) {
    return (overrides: DeepPartial<T>) => {
        // here we can inject faker that is passed via `options.faker` that way you don't need to import faker all over the place
        const _defaults = unwrapThunk(options.faker, defaults);
    }       
  }
}
// Module to define your factory
import {faker} from '@faker-js/locales/de';

// other options could include things like `probability`, `mergeStrategy`, etc.
const germanFactory = createMockFactory({faker, ...otherOptions});


// Module where you create a mock
interface Person {
  firstName: string;
  phone: number;
}

const mock = germanFactory<Person>((f) => ({
  firstName: f.name.firstName(),
  phone: f.datatype.number(),
});

Another interesting possibility would be to infer via strings or even auto infer based on name.

In the case above, we are using firstName, what if the mock just knew how to mock that based on similarity to the name, or there was a babel plugin to auto generate factories based on comments.

interface Person {
  // @faker-method:faker.name.firstName
  firstName: string;
  // @faker-method:faker.datatype.number
  phone: number;
}

would result in a method

const mockPerson = createMockFactory<Person>(() => ({
  firstName: faker.name.firstName(),
  phone: faker.datatype.number(),
}));

How to move the merge strategy to user land?

How about the merge method/strategy. You raise a good point:

usecase dependent and thus would require a parameter to configure it. However the parameter would most likely contain the actual implementation of the merge, that would otherwise just be in the method itself.

Though I think you could expose a configurable merge strategy without totally exposing the merge functionality. I've been experimenting with this idea where you pass the overrides into the default function allowing a user to compare and do whatever they want with the result. Here is an example.

interface Money {
  currency: CurrencyCode;
  amount: number;
}

interface PriceRange {
  min: Money;
  max: Money;
}

const mockMoney = createMockFactory<Money>(() => ({
  currencyCode: faker.helpers.arrayElement<CurrencyCode>(Object.values(CurrencyCode)),
  amount: faker.datatype.number({min: 0, max: 100, precision: 2}),
});

const mockPriceRange = createMockFactory<PriceRange>((overrides) => {
  let min = mockMoney();
  let max = mockMoney({amount: faker.datatype.number({min: min.amount + 1}), currencyCode: min.currencyCode});

  // in the case where we only pass a min value, we should make the max value 10x
  if (overrides.min && !overrides.max) {
    return {min, max: mockMoney({amount: min.amount * 10, currencyCode: min.currencyCode});
  }

  return {min, max};
});

and Usage:

const withMin = mockPriceRange({min: {amount: 100}}); // we know max is 1000
const withMax = mockPriceRange({max: {amount: 100}}); // min is randomly defined

This gets interesting with arrays

const mockGroup = createMockFactory<Group>((overrides) => {
  
  // when we pass members in the overrides, define the default members using a static catalog that is the same length
  if (Array.isArray(overrides.members)) {
    const catalogOfPeople = staticPeopleList.filter((member) => !catalogOfPeople.map(({firstName}) => firstName).includes(member.firstName)
    return {
      // exclude members in the default array that have the same firstName as members in the overrides array
      members: faker.helpers.arrayElements<Person>(catalogOfPeople, overrides.members.length)
    };
  }

  // if we don't specify any members, then return empty array. we could have any logic we want. userland gets to decide everything
  return {members: [], id: faker.datatype.uuid()};
});

And usage:

// empty members is always empty
expect(mockGroup().members).toStrictEqual([]);
// we end up with a member list 2x length of our override
expect(mockGroup({members: [member1, member2]}).members.length).toEqual(4);
// we know that all member names are unique
expect(new Set(mockGroup({members: [member1, member2]}).members.map((member) => member.firstName)).length).toEqual(4);

This (if I'm not mistaken) should allow virtually infinite merge strategies and doesn't force us to expose the logic or algorithm used for actually merging the defaults and the overrides. In the end, the default functions above will return an object, that is then going to be merged with the default. (I know that this would require a different approach to the internal merge functionality than what I originally have, but like I said, this is WIP) In this approach we would likely have to use a straight merge because now we are allowing the user to specify the default value based on the override, and ultimately we should just merge the result.

Yet another variation of this approach would be to have the defaults return the full value, and just skip the merge. Instead of merging the defaults and overrides we just pass the overrides into the defaults function and return the final result. This might actually be the best approach as it balances the flexibility without adding complexity. Would need to think about the tradeoffs more though and would love input.

@ST-DDT
Copy link
Member

ST-DDT commented Nov 6, 2022

Overrides and Merging

If we tried to implement a concept of overrides into createUser (like you have in some of your examples) we would just end up implementing something very similar to what i'm already proposing.

Yes, that the overrides option for createUser is intentional, as it shows how easy that is to implement yourself, no need for a special factory method that does things that are hard to read/spread over multiple places.

It's fair to say you might have a preference for setting values via createUser over, specifying values via mockUser. That's an opinion about how you want to define data on an object, which is fair.
[...]
using mockGroup you can go both ways

This is not really about specifying vs setting, as that is just an implementation detail IMO as long as you have the override option you can do it one way or the other.
Implementing the override feature is quite easy as shown my previous examples, it comes for free if you use the mockX approach as long as you do it "the default way". If it is not the default way, it is probably harder to implement than in the createX approach.
I assume that the default way of replacing arrays and merging objects would cover most of the usecases, so it might be a slight improvement for most users of that feature.

Optional Properties

by restricting optional properties we can be 100% sure what the shape of a final mock object will be just by looking at what values are required and what you pass into the overrides

IMO it should be other way round. You are testing something that probably a user will specify, so you don't know the exact shape of the object. It might have the optional property or not, you don't know. The same is true for test data.
If you are testing a specific case, then you explicitly set the attributes/shape you need everything else remains unknown.

Lets say you add a new optional property to your datamodel that affects something worth checking.
If you skip it by default you don't have to change any of your tests to accomodate it, but likewise you don't know whether it impacts any of your tests if it is present. You now have to effectively duplicate all your tests for each optional property to ensure that it does not have an impact on any of your other features if present. If it is random/unknown, then you don't have to do that because eventually you will have or not have a value for that property and your tests will show a breakage/fail. This gets more important if you have more optional features or more than one person working on that particular component as the developer might no longer know how each property affects the output. Especially quick bugfixes or late feature additions usually lack the insider knowledge that went into the creation of the component.

If we allow the methods to contain optional properties, then the user can decide whether it should include that property or not.
So if we don't want to limit users to one way, we have to use the more lenient way.
Maybe we could add an additional generic type parameter to the method that can be used to define the generated shape.
e.g. createMockFactory<Type, Shape = Type>(...) => createMockFactory<User, OnlyRequiredProperties<User>>(...)

Use Faker in the Implementation

  1. If we included the logic for conditionally providing optional values per your feedback you could use faker helpers for that.

I'm not sure what you are referring to here. IMO there is no difference between calling faker.person.firstName and calling faker.helpers.maybe. Also AFAICT there is no programatic way to check if a property is optional or not, so you have to actually implement that yourself.

  1. I like the idea of injecting faker into the default value function or even improving DX for defining some values

IMO passing a faker instance to the factory (creator) or when using the factory is actually an interesting idea, although you could just pass it to a method almost identically. One of the benefits here is actually in combination with #1499 faker.derive that ensures that only a single value is taken from the seed and thus the methods would produce the same values even if a previous method generate an additional property.
This might be useful for our later "tree-shakeable module functions", although it wouldn't need any overrides/deep merge logic.

export function firstName({ faker: Faker, sex?: SexType }) {
   [...]
}
// ---
export function bind<Params, Result>(fn: ({faker: Faker} & Params) => Result, faker: Faker): (options: Params) => Result {
    return (options) => fn({...options, faker});
}

We will discuss this concept in our next team meeting again.

As for generating a factories from annotations, this is an entirely different proposal.

Merge Strategies

Though I think you could expose a configurable merge strategy without totally exposing the merge functionality. I've been experimenting with this idea where you pass the overrides into the default function allowing a user to compare and do whatever they want with the result.

So we are basically back to the method without factory?

export function createUser(overrides: DeepPartial<User>, options: { includeOrganization?: boolean } = {}) {
    const { includeOrganization } = options;
    return {
           name: faker.internet.userName(),
           ...overrides,
           organization: faker.helpers.maybe(() => createOrganization(overrides.organization), includeOrganization);
    };
}

I myself would like to see your ideas for a createMockFactory / createObjectFactory and/or a createValueFactory (that can be used for primitives as well, but doesn't have overrides support).

Here are some (somewhat soft) requirements for the two methods:

  • take a faker instance as an argument
  • have a type parameter and a "type shape" parameter (e.g. <User, OnlyRequiredProperties<User>>)
  • support overrides for the resulting method
  • support options/params for the resulting method
  • more requirements might be added later (my brain is too exhausted after spending 3 hours on this)

Please be aware, that the PR is currently unlikely to be merged at all, might require heavy rewrites / evolutions before it gets merged, might only be partially be merged or only some concepts get merged and might stay unmerged for a long time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: docs Improvements or additions to documentation p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

2 participants