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

Models instantiated in Djangos setUpTestData seem to skip conditions #424

Closed
robinharms opened this issue Mar 28, 2024 · 5 comments · Fixed by #425
Closed

Models instantiated in Djangos setUpTestData seem to skip conditions #424

robinharms opened this issue Mar 28, 2024 · 5 comments · Fixed by #425

Comments

@robinharms
Copy link

  • Python State Machine version: 2.1.2
  • Python version: 3.12
  • Operating System: OsX 12.7.2
  • Django: 5.0.3

Description

The MachineMixin for Django seems to behave differently when models are used within django tests.
Oddly enough, any objects created during setUpTestData seem to cause transitions to bypass checks like
conditions.

What I Did

This is a simple django application. Create migrations, run tests.
(I'm skipping some imports for stuff you probably know)

statemachines.py

class MySM(StateMachine):
    draft = State("Draft", initial=True, value="draft")
    published = State("Published", value="published")

    publish = draft.to(published, cond="let_me_be_visible")

models.py

class MyContext(models.Model, MachineMixin):
    state = models.CharField(max_length=30, blank=True, null=True)
    state_machine_name = "myapp.statemachines.MySM"
    state_machine_attr = "wf"
    let_me_be_visible = False

tests.py

from django.test import TestCase
from statemachine.exceptions import TransitionNotAllowed

from myapp.models import MyContext
from myapp.statemachines import MySM


class WFTests(TestCase):
    @classmethod
    def setUpTestData(cls):
        # Runs once
        cls.one = MyContext.objects.create()

    def setUp(self):
        # Runs before every test
        self.two = MyContext.objects.create()

    def test_one(self):
        # This test will fail
        with self.assertRaises(TransitionNotAllowed):
            self.one.wf.send("publish")

    def test_two(self):
        # This works as expected
        with self.assertRaises(TransitionNotAllowed):
            self.two.wf.send("publish")

    def test_three(self):
        # Managing this instance works if I call it like this instead.
        # So this test works
        wf = MySM(self.one)
        with self.assertRaises(TransitionNotAllowed):
            wf.send("publish")

The inner workings of the transitions are still a bit opaque to me, so sorry if there's something obvious I'm missing here.

Thanks for maintaining this great library!

@fgmacedo
Copy link
Owner

Hi @robinharms, how are you? I didn't get time to work on this. Do you still have this issue?

Given the cost of setup to reproduce the issue, could you please provide me with a toy repository that I could quickly jump into and debug the error?

@robinharms
Copy link
Author

Sure no problem, I can't do it right away but I'll try to get it done tomorrow!

@robinharms
Copy link
Author

Here you go!
https://github.com/robinharms/pysm-django

Just ask if there's anything else I can do. Thanks a lot for this!

@fgmacedo
Copy link
Owner

fgmacedo commented Apr 16, 2024

Thanks, @robinharms! I was able to reproduce the issue locally.

Here's what I've found: The root cause of the issue lies in the setUpTestData() method. Every cls attr created in this method is wrapped in a TestData class, which performs a deepcopy() for each test, so this is designed to allow safe alteration of objects between tests on the same TestCase instance.

However, this conflicts with the creation strategy of state machines. The references of properties/methods used as actions/conditions are computed at the time of creation. So, when the state machine is cloned, the references still point to the original instance.

I'm currently exploring potential solutions to this problem, if any.

@robinharms
Copy link
Author

Nice! Thanks a lot for this! 😄

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 a pull request may close this issue.

2 participants