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

Tests: make factories default to populating dependent objects #1944

Closed
wants to merge 1 commit into from

Conversation

singingwolfboy
Copy link
Contributor

@singingwolfboy singingwolfboy commented Dec 2, 2016

The Factory Boy library, which we're using for making test data, has some nice utilities for automatically populating dependent objects in a data model. In particular, we can use SubFactory to create a dependent object before the target object is finished being created, and we can use RelatedFactory to create a dependent object after the target object is finished being created. In both cases, it's easy to indicate that the dependent object shouldn't be created at all, simply by passing <dependent>=None as keyword arguments when creating the target object.

For example, when creating a Program object, you'll probably also want to create a Course object for it, and a CourseRun object for the Course, and perhaps even a CoursePrice object for the CourseRun. Previously, this was handled by passing a full=True parameter to the ProgramFactory class, which was detected by overriding the _create() method. Now, creating these dependent objects is the default usage. If you don't want to create a Course object, you can call ProgramFactory.create(course=None), and you'll just get a Program object. If you want a Program and a Course but not a CourseRun, you can call ProgramFactory.create(course__course_run=None). You'll still get a Program object, and a Course object will be created for you, but a CourseRun object will not be created.

This pull request is still in progress, because there are a few tests in the codebase that need to be changed to accommodate this new assumption. However, it's mostly working already.

@odlbot odlbot temporarily deployed to micromasters-ci-pr-1944 December 2, 2016 16:08 Inactive
@gsidebo
Copy link
Contributor

gsidebo commented Dec 2, 2016

currently using this in an open PR: #1916; more specifically, https://github.com/mitodl/micromasters/pull/1916/files#diff-b92d8d54bd10dd2713c797bdffeb0b90R41

surely there's some way that we can keep something around that accomplishes the goal of this code (reduce the large amount of boilerplate to create a program/course/course run with ancillary records like course price/tier program where needed) instead of removing it entirely

@gsidebo
Copy link
Contributor

gsidebo commented Dec 2, 2016

maybe creating a different factory class for the 'full' Program factory is more easily understood and implemented

@codecov-io
Copy link

codecov-io commented Dec 2, 2016

Current coverage is 93.93% (diff: 100%)

No coverage report found for master at 737de16.

Powered by Codecov. Last update 737de16...414ba2a

@odlbot odlbot temporarily deployed to micromasters-ci-pr-1944 December 2, 2016 18:03 Inactive
@odlbot odlbot temporarily deployed to micromasters-ci-pr-1944 December 2, 2016 18:43 Inactive
@singingwolfboy singingwolfboy force-pushed the remove-program-factory-full-create branch from 8e380d8 to 352563b Compare December 6, 2016 17:42
@odlbot odlbot temporarily deployed to micromasters-ci-pr-1944 December 6, 2016 17:42 Inactive
@odlbot odlbot temporarily deployed to micromasters-ci-pr-1944 December 6, 2016 17:49 Inactive
@odlbot odlbot temporarily deployed to micromasters-ci-pr-1944 December 6, 2016 18:02 Inactive
@singingwolfboy singingwolfboy force-pushed the remove-program-factory-full-create branch from 8b6ecf2 to 96a5015 Compare December 6, 2016 18:04
@odlbot odlbot temporarily deployed to micromasters-ci-pr-1944 December 6, 2016 18:04 Inactive
@gsidebo
Copy link
Contributor

gsidebo commented Dec 6, 2016

@singingwolfboy what's going on with this pull request? did you see my comments above? also, there's a bunch of work around the ProgramPage model which seems to indicate that this PR has shifted purpose, so to speak

@singingwolfboy singingwolfboy changed the title Remove program factory full create in tests Tests: make factories default to populating full relationships Dec 6, 2016
@singingwolfboy singingwolfboy changed the title Tests: make factories default to populating full relationships Tests: make factories default to populating dependent objects Dec 6, 2016
@singingwolfboy
Copy link
Contributor Author

@gsidebo Yes, I saw your comment -- sorry for not responding. I started exploring ways to make this work more cleanly, and I learned about Factory Boy's RelatedFactory attribute, which solves this case very neatly. I've updated the description of this pull request to make it more accurate. Does that make the situation more clear?

@singingwolfboy singingwolfboy force-pushed the remove-program-factory-full-create branch from a0f6dcf to 414ba2a Compare December 6, 2016 20:48
@odlbot odlbot temporarily deployed to micromasters-ci-pr-1944 December 6, 2016 20:48 Inactive
course_price = CoursePriceFactory.create(course_run=course_run, is_valid=True)
if program.financial_aid_availability:
from financialaid.factories import TierProgramFactory
@factory.post_generation
Copy link
Contributor

Choose a reason for hiding this comment

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

see, this is what i really wanted when i wrote that full=True nonsense :) thanks for finding this! i'll try to get to this PR today

@singingwolfboy
Copy link
Contributor Author

This ended up being more work than I anticipated, for comparatively little benefit. I'm going to close this pull request, but leave the branch around at least for now. If we want to pick this up again in the future, we can do so.

@pdpinch pdpinch deleted the remove-program-factory-full-create branch July 18, 2018 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants