Skip to content

Update test factories #6335

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Sep 28, 2021
Merged

Update test factories #6335

merged 6 commits into from
Sep 28, 2021

Conversation

smotornyuk
Copy link
Member

@smotornyuk smotornyuk commented Aug 19, 2021 •
edited
Loading

Modernize test factories and add factoryboy fixtures.

New features:

  • factories are based on factoryboy's SQLAlchemy factory
  • new fixtures
  • few sentences in documentation with explanation of how to create new factory(inside extension)
  • titles, passwords, names, etc. are random now(using faker instead of number-sequences)

Docs: factoryboy SQLAlchemy and pytest-factoryboy

Examples

# random user
def test():
    user = factories.User()

# turns into

def test(user):
   # user == factories.User()
   pass

# user with controlled properties
def test():
    user = factories.User(name="xxx")

# turns into

@pytest.mark.parametrize("user__name", ["xxx"])
def test(user):
   pass

# or

def test(user_factory):
   user = user_factory(name="xxx")

# getting User model instead of user_dict
def test():
    user_dict = factories.User(name="xxx")
    user_obj = model.User.get(user_dict["id"])

# turns into

def test(user_factory):
   user_obj = user_factory.model(name="xxx")


# if you want to generate model object that is not flushed into DB
def test():
    attrs = factories.User.attributes()
    user_obj = model.User(**attrs)
    assert user_obj.id is None

# turns into

def test(user_factory):
    user_obj = user_factory.build()
    assert user_obj.id is None

@smotornyuk smotornyuk assigned smotornyuk and unassigned smotornyuk Aug 19, 2021
@pdelboca pdelboca mentioned this pull request Aug 26, 2021
Closed
amercader
amercader reviewed Sep 10, 2021
View reviewed changes
ckan/tests/controllers/test_admin.py Outdated
@@ -233,10 +228,10 @@ def test_trash_view_sysadmin(self, app, sysadmin_env):
# On the purge page
assert "purge-all" in trash_response

def test_trash_no_datasets(self, app, sysadmin_env):
def test_trash_no_datasets(self, app, sysadmin_env, package_factory):
Copy link
Member

Choose a reason for hiding this comment

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

Can we call it dataset_factory? 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's possible.

I used "package" because I want to highlight that this dataset is not the only possible package type. In their own projects, people should create sub_factories for creating package factories with the non-dataset type.

And for dataset type, if the developer customizes it, then he can register a specific dataset_factory fixture, with the package[type=dataset] that has set of fields sensible for this particular portal's dataset. While package_factory is still available if he needs something more low-level

@amercader
Copy link
Member

I know this is a matter of personal taste but I find more readable calling the factories explicitly to create instances that using the factory fixture, eg not sure than this:

def test(user_factory):
   user = user_factory(name="xxx")

is more readable than this:

def test():
    user = factories.User(name="xxx")

(I really don't like the @pytest.mark.parametrize("user__name", ["xxx"]) variant)

I specially don't like it in cases where you need to pass a lot of fixtures to set up several factory instances, like here and I can imagine people being confused about whether to import the organization or organization_factory fixture.

But perhaps I'm focusing too much on the syntax and missing the bigger picture here. How do these changes align with your wider work in tests? Do we need these changes to enable us something down the line?

@smotornyuk
Copy link
Member Author

smotornyuk commented Sep 10, 2021 •
edited
Loading

They are absolutely optional and I'm going to revert this particular piece (using fixtures instead of explicit factories).

It's more an example of how people can(if they want) write tests. As for core tests - I prefer everyone to be comfortable with codebase much more than using fancy solutions :)

I was thinking about different approaches and at the moment I can see a way of increasing speed without test factories.

But I think, that:

  • Extending SQLAlchemyFactory + factory.model method that returns model instance after creating the entity are good things, to they should be included
  • factory fixtures can be registered just as now, so that anyone who wants, can use them. What is actually bad in this PR - rewriting factory.Class to the class_factory - we should leave this piece unchanged.

Is it better?

@smotornyuk
Copy link
Member Author

Hi, @amercader

I've removed these mass changes from factories to fixtures. How does it feel now?

  • fixtures are still there and are available for users, I just didn't replace factories all over the codebase

@amercader
Copy link
Member

@smotornyuk Sorry it took a while to revisit this, it's looks good to me.

@amercader amercader merged commit f52151f into master Sep 28, 2021
@amercader amercader deleted the update-factories branch September 28, 2021 10:08
@TomeCirun TomeCirun mentioned this pull request Sep 28, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amercader amercader amercader left review comments

Assignees

@wardi wardi

@amercader amercader

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smotornyuk @amercader @wardi