thoughtbot / fishery

A library for setting up JavaScript objects as test data
MIT License
870 stars 36 forks source link

I’d Love To See an `attributesFor` Option Like in `factory_bot` #120

Closed klondikemarlen closed 1 year ago

klondikemarlen commented 1 year ago

Is your feature request related to a problem? Please describe. I'd like to be able to do userFactory.attributesFor() so that I can conveniently test services and such. Currently I'm using Sequelize so userFactory.build() returns a new Sequelize model instance - which is nice! - but when I have a user creation service, I need to build the model, then extract the attributes before passing them to my service, which looks like this

const { dataValues: attributes } = userFactory.build()
// or const attributes = { some attributes ... } 
// and I usually use this form, but I still generate the random data with `fisheries` + `jsFaker` and `.build().dataValues`
// I just miss the beauty of the Ruby version :)

// creating the user if valid, and performing a bunch of stuff around user creation, like logging and sending emails, etc. 
expect(() => await UserServices.create(attributes, { currentUser })).toChange(() => User.count).by(1)
// other tests

Describe the solution you'd like I'd love to be able to separate Sequelize model building from attribute generation.

Describe alternatives you've considered I've looked into building a base factory that adds a buildModel method that wraps the build method in a Sequelize model. It works, just isn't as clean as the Ruby version.

Additional context I love the Ruby version of this package, and still definitively like this TS/JS version more than any of the other TS/JS alternatives.

stevehanson commented 1 year ago

Hi, @klondikemarlen. I'm glad you are finding the library useful! You mentioned an alternative of creating a custom buildModel method on your factory, and that is probably what I would recommend for now. Alternatively, you could keep your current approach where the factory returns your Sequelize model, and then add a buildAttributes method on your factory. Here is what that could looks like:

import { DeepPartial, Factory } from "fishery";
import { Sequelize, DataTypes, Model } from "sequelize";
const sequelize = new Sequelize("sqlite::memory:");

class User extends Model {
  declare id: number;
  declare name: string | null;
}

User.init(
  {
    id: {
      type: DataTypes.INTEGER,
      allowNull: false
    },
    name: {
      type: DataTypes.STRING
    }
  },
  { sequelize }
);

class UserFactory extends Factory<User> {
  // add a custom buildAttributes method!
  buildAttributes(params: DeepPartial<User>) {
    return this.build(params).dataValues;
  }
}

const userFactory = UserFactory.define(() =>
  User.build({
    id: 1,
    name: "Susan"
  })
);

const attributes = userFactory.buildAttributes({ id: 2 });

expect(attributes).toMatchObject({
  id: 1,
  name: "Susan"
});

You could even generalize this with a SequelizeFactory that you extend for all of your models:

class SequelizeFactory<T extends Model> extends Factory<T> {
  // add a custom buildAttributes method!
  buildAttributes(params: DeepPartial<T>) {
    return this.build(params).dataValues;
  }
}

class UserFactory extends SequelizeFactory<User> {}
const userFactory = UserFactory.define(() => User.build({ ... }))
userFactory.buildAttributes({ name: 'Susan' })

At this time, I don't plan to add anything for this in the library itself, since everyone's situation is different in how they build objects.

klondikemarlen commented 1 year ago

Interesting ... I didn't realize you could do it this way, though it retrospect it makes sense.

class UserFactory extends Factory<User> {
  // add a custom buildAttributes method!
  buildAttributes(params: DeepPartial<User>) {
    return this.build(params).dataValues;
  }
}

const userFactory = UserFactory.define(() =>
  User.build({
    id: 1,
    name: "Susan"
  })
);

I'll probably just build out a base class with this method then .. Thanks for the quick, and comprehensive response!

klondikemarlen commented 1 year ago

For future reference I built out a BaseFactory class that adds the appropriate methods. I'm sure there is a better way to extend typescript types, but I'm pretty new to them so its just a clone of the fisheries code, and probably shouldn't exist in a production environment.

// base-factory.ts
import { BuildOptions, DeepPartial, Factory, GeneratorFn } from "fishery"
import { Model } from "sequelize"

// See https://github.com/thoughtbot/fishery/blob/2bd552c8185bfce29c90faa09dd2a576e6282663/lib/factory.ts#L15
export class BaseFactory<T, I = any, C = T> extends Factory<T, I, C> {
  // See https://github.com/thoughtbot/fishery/blob/2bd552c8185bfce29c90faa09dd2a576e6282663/lib/factory.ts#L37C3-L42C4
  static define<T, I = any, C = T, F = BaseFactory<T, I, C>>(
    this: new (generator: GeneratorFn<T, I, C>) => F,
    generator: GeneratorFn<T, I, C>
  ): F {
    return new this(generator)
  }

  // See https://github.com/thoughtbot/fishery/blob/2bd552c8185bfce29c90faa09dd2a576e6282663/lib/factory.ts#L49C28-L49C28
  buildAttributes(params?: DeepPartial<T>, options: BuildOptions<T, I> = {}): T {
    const model = this.build(params, options) as Model
    return model.dataValues
  }

  // See https://github.com/thoughtbot/fishery/blob/2bd552c8185bfce29c90faa09dd2a576e6282663/lib/factory.ts#L53
  buildAttributesList(
    number: number,
    params?: DeepPartial<T>,
    options: BuildOptions<T, I> = {}
  ): T[] {
    const list: T[] = []
    for (let i = 0; i < number; i++) {
      const model = this.build(params, options) as Model
      list.push(model.dataValues)
    }

    return list
  }
}

export default BaseFactory
stevehanson commented 1 year ago

@klondikemarlen thanks for circling back to share what you came up with for others who land here!

The only thought I'd add is that you could type the T generic param as T extends Model in the class definition (i.e. class BaseFactory<T extends Model, ...>. This would ensure this factory would only every be used to build Models and remove the need for those as Model casts.

I'm also curious why the define function is necessary in BaseFactory. Was it so you could pass the C generic param through? That's interesting. Ideally, you'd not have to override that.

Thanks again for sharing!

klondikemarlen commented 1 year ago

For whatever reason (I'm still pretty new to Typescript) T extents Model needs to be T extends Model<???> but the attributes passed are complicated, so I cast with as Model instead.

I needed to override define as well because Factory.define returns a Factory instance, and ... BaseFactory.define also returns a Factory instance, so it doesn't believe it has the buildAttributes method. I needed to replace static define<T, I = any, C = T, F = Factory<T, I, C>>( with static define<T, I = any, C = T, F = BaseFactory<T, I, C>>(. Note the change from F = Factory to F = BaseFactory.

I wanted the simplest possible usage for child factories. e.g.

// factories/user-factories.ts
import { faker } from "@faker-js/faker"

import { User } from "@/models"
import { BaseFactory } from "@/factories"

export const userFactory = BaseFactory.define<User>(({ sequence, onCreate }) => {
  onCreate((user) => user.save())

  return User.build({
    email: `${faker.internet.email()}-${sequence}`,
    ...
  })
})

export default userFactory

Which gives

userFactory.buildAttributes() // returns object attributes
userFactory.buildAttributesList() // returns list of object attributes
userFactory.build() // returns model instance
userFactory.create() // returns saved model instance