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

Make _setup_next_sequence usable with DictFactory subclasses #1056

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

danjer
Copy link

@danjer danjer commented Dec 29, 2023

Add new option class for DictFactory that checks for the implementation of _setup_next_sequence to determine counter reference.

Fixes #993

Copy link
Member

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for taking the time to contribute a PR, with tests. 😁

Sadly, the suggested approach introduces a difference in behavior between DictFactory and the other factories.

DictFactory sets a model in its options, which results in all DictFactory subclasses sharing the same base factory (and reference_counter): DictFactory. Hence, only the _setup_next_sequence() from DictFactory is executed, and it returns 0. That is undesirable, but that’s the current behavior.

With the suggested patch, each factory in the dict hierarchy can reset its sequence, so you can make a subclass of Dog and have it define a _setup_next_sequence() to start from 0. Whereas, for other factories, all subclasses use the same sequence as the top-most factory that defines a model in the inheritance. To illustrate, the following test does not pass:

    def test_sequence_resets_on_subclass(self):
        class Base42Factory(factory.Factory):
            class Meta:
                model = TestObject

            @classmethod
            def _setup_next_sequence(cls):
                return 42

            one = factory.Sequence(lambda n: f'{n}')

        class Base9000Factory(Base42Factory):
            @classmethod
            def _setup_next_sequence(cls):
                return 9000

        base9000 = Base9000Factory.build()
        self.assertEqual('9000', base9000.one)  # Is '42'.

        base42 = Base42Factory.build()
        self.assertEqual('42', base42.one)

Possible ideas:

  1. follow the existing practice, maybe special-casing the DictFactory so that the reference_counter points to the top-most factory beneath DictFactory in the inheritance.
  2. allow resetting sequences for all factories.

I think the second would make for a nicer behavior, but that requires heavier changes to how the library works (and updating the docs to clearly define the behavior).

def test_descendants_share_counter(self):
cat_result = self.Cat()
dog_result = self.Dog()
results = [cat_result['pet_id'], dog_result['pet_id']]
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this list is necessary? Why not directly assert the cat_result['pet_id'] and dog_result['pet_id'] ?

@francoisfreitag francoisfreitag changed the title Fix#993 Make _setup_next_sequence usable with DictFactory subclasses Jan 16, 2024
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.

DictFactory does not override _setup_next_sequence
2 participants