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

NEW: Allow SapphireTest::objFromFixture() to accept either table or class #6872

Merged
merged 1 commit into from May 5, 2017

Conversation

sminnee
Copy link
Member

@sminnee sminnee commented May 4, 2017

Right now SapphireTest::objFromFixture() requires a class as the first
argument. This is fine when your fixture file uses classes as the keys,
but if populating a fixture via tables, objFromFixture() won’t work.

This patch lets you specify either the table name or the class name as
the key.

The benefit here is that you can build fixtures as raw inserts, which is
substantially quicker, and is likely to be a useful tool in building
more efficient test suites.

sminnee pushed a commit to sminnee/silverstripe-blog that referenced this pull request May 4, 2017
@sminnee
Copy link
Member Author

sminnee commented May 4, 2017

This came up when I was looking into why blog tests were so slow. It had the following improvements:

  • Fixture set-up time: 4s => 0.08s
  • Text execution time: 170 seconds => 30 seconds.

You can see what I had to change here: silverstripe/silverstripe-blog@c4abe91

The yaml is admittedly more verbose but a reasonable execution time on tests is worth the effort. Longer term we might want to find alternatives to yml fixtures as they encourage some bad habits, but that's for another day.

…lass

Right now SapphireTest::objFromFixture() requires a class as the first
argument. This is fine when your fixture file uses classes as the keys,
but if populating a fixture via tables, objFromFixture() won’t work.

This patch lets you specify either the table name or the class name as
the key.

The benefit here is that you can build fixtures as raw inserts, which is
substantially quicker, and is likely to be a useful tool in building
more efficient test suites.
@tractorcow
Copy link
Contributor

The benefit here is that you can build fixtures as raw inserts, which is substantially quicker

Yes but it also bypasses all insert hooks, doesn't populate subtables, versions table, etc and bypasses error checking and validation. My feeling is that this introduces risk of errors silently creeping into test state. How do you feel about this?

@sminnee
Copy link
Member Author

sminnee commented May 5, 2017

Yes but it also bypasses all insert hooks, doesn't populate subtables, versions table, etc and bypasses error checking and validation. My feeling is that this introduces risk of errors silently creeping into test state. How do you feel about this?

I think that in many cases the benefits outweigh the costs, and that the costs are not so great that we should make the approach I've suggested impossible.

As an example: it won't be feasible to have code coverage for the blog module without this.

@sminnee
Copy link
Member Author

sminnee commented May 5, 2017

In the big picture I think I'd like to:

  • Investigate the creation time of SiteTree to see why they're so much slower than raw inserts
  • Move away from using fixture files as they encourage the creation of "kitchen sink" fixtures that slow down creation-time and instead have a DSL for scaffolding data on a per-test-method basis

However both of these are more work than the improvement allowed by this patch.

@tractorcow tractorcow merged commit d2c6c53 into silverstripe:master May 5, 2017
@tractorcow tractorcow deleted the fixture-by-table branch May 5, 2017 03:14
sminnee pushed a commit to sminnee/silverstripe-blog that referenced this pull request May 11, 2017
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.

None yet

2 participants