thoughtbot / fishery

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

Using any of the factory extension methods breaks sequence #49

Closed ddavisso4 closed 3 years ago

ddavisso4 commented 3 years ago

Using any of the extension methods (params, associations, etc.) seems to causes the sequence to stop incrementing. I'm wondering if this is by design or not and if there is a recommend approach. I see you can explicitly call it by extending Factory but this creates a situation where I either have to know if given function increments the sequence or remember to include it in each function to be safe. I have a feeling that this has something to do with the clone implementation but figured I'd ask before I got too far. Thanks in advance!

Here's an example...

interface Model {
  id: number,
  prop1: number,
  prop2: string
}
class ModelBuilder extends Factory<Model> {
  withSameValue(prop: number) {
    return this.params({
      prop1: prop,
      prop2: prop.toString()
    });
  }
}

export const modelBuilder = ModelBuilder.define(({sequence}) => ({
  id: sequence,
  prop1: 10,
  prop2: "test"
}));

The following code...

const model1 = modelBuilder.withSameValue(5).build();
console.log(model1);
const model2 = modelBuilder.withSameValue(6).build();
console.log(model2);

...outputs...

LOG LOG: Object{id: 1, prop1: 5, prop2: '5'}
LOG LOG: Object{id: 1, prop1: 6, prop2: '6'}

The following code...

const model1 = modelBuilder.build();
console.log(model1);
const model2 = modelBuilder.build();
console.log(model2);

...outputs...

LOG LOG: Object{id: 1, prop1: 10, prop2: 'test'}
LOG LOG: Object{id: 2, prop1: 10, prop2: 'test'}
BenEinwechter commented 3 years ago

I ran into this same issue. From my investigation, it appears each call to an extension method (params, associations, etc) returns a new instance of a factory object, and since sequence is an instance variable on a factory object, the sequence is not shared between the two factories the extension method returns.

Here's a pseudocode illustration:

const factoryInstance1 = modelBuilder.withSameValue(5)
// factoryInstance1.sequence is 1

const factoryInstance2 = modelBuilder.withSameValue(6)
// factoryInstance2.sequence is 1

const model1 = factoryInstance1.build()
// factoryInstance1.sequence is now 2, but factoryInstance2.sequence is still 1

The way I worked around this limitation is by creating my own sequencer instance that can be used by any factory:

(warning: might be some syntax errors since I don't have the actual code with me at the moment)

// sequencer.js
export function buildSequencer(key) {
  // global variable to track sequences
  const sequences = {}

  // closure over sequences
  return function (key) {
    if (!sequences[key] {
      sequences[key] = 1;
    }
    const current = sequences[key];
    sequences[key] += 1;
    return current;
  };
}

Then inside a factory I can use it like this:

import { buildSequencer } from './sequencer'

const nextSequence = buildSequencer();

const projectFactory = Factory.define(() => ({
  name: `Project ${nextSequence('project')}`
}));

Now even if I add extender methods to the projectFactory, all the factory instances be using the same sequencer so my project name's will be incremental unique across the whole test run.

stevehanson commented 3 years ago

Thank you for submitting this issue.

@BenEinwechter, you are correct that factories are not designed to share sequences and that extending factories using the builders creates new instances of the factories altogether with independent sequences.

With that said, I could definitely understand a case for having the sequences be shared between these factories. I am adding an item to our agenda to consider this behavior and then will either close this issue with a pull request or close this as something we are not yet planning to implement. In the meantime, you could always roll your own mechanism for sequencing as @BenEinwechter demonstrated.

ddavisso4 commented 3 years ago

@BenEinwechter thanks for the tip. I had a similar idea but figured I was just missing something. Your solution looks great! @stevehanson thanks for considering and I'll see about a PR. (1) tree house for the girls, (2) fishery PR.

stevehanson commented 3 years ago

@ddavisso4 definitely start with the tree house 🙂

I have looked into this and decided that we should fix this issue. Since the extension methods are used to define traits on factories, it's not intuitive that the sequences would diverge each time the extension methods are used.

I have written a failing test and will try to get a PR together on Friday. Thanks again for reporting this!

stevehanson commented 3 years ago

Thanks for the review and for reporting this, @ddavisso4. I've merged and will plan to get a build out sometime this week.