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: Use a videos fixture instead of seeding videos.csv #27788

Merged
merged 2 commits into from Apr 1, 2019

Conversation

islemaster
Copy link
Contributor

@islemaster islemaster commented Mar 30, 2019

Use a videos fixture file instead of seeding videos from videos.csv for dashboard unit tests. Follow-up work to #27780, see that PR for original context.

Will mentioned on Friday that we could stop depending on videos.csv entirely and add a videos fixture for tests instead. I thought this was worth a try.

Concerns

One concern that's been raised is that testing with videos.csv does actually catch issues. I'm not worried about this, because the same validation step that caused problems in the unit tests Friday is also run as part of the regular rake seed:videos task, so these issues should be caught even if we're using fixtures for tests.

Another: I see more than 90 different video keys referenced in our level.yml fixture file. We've only got 340 rows in videos.csv. If I end up needed to add fixtures for all of those videos to get tests passing, is it worth maintaining video fixtures separate from our videos.csv file at all?

See also: https://api.rubyonrails.org/v3.1/classes/ActiveRecord/Fixtures.html

Use a videos fixture file instead of seeding videos from videos.csv for
dashboard unit tests.  Follow-up work to
#27780.

See also https://api.rubyonrails.org/v3.1/classes/ActiveRecord/Fixtures.html
Copy link
Contributor

@wjordan wjordan left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the followup on this!


Another: I see more than 90 different video keys referenced in our level.yml fixture file. We've only got 340 rows in videos.csv. If I end up needed to add fixtures for all of those videos to get tests passing, is it worth maintaining video fixtures separate from our videos.csv file at all?

From what it looks like you managed to get the tests passing with just 3 fixtures, so maybe this concern is now moot. In any case, our current script-model fixtures are generated from a script that produces them from a subset of source data. This helps separate the unit-test process from the process of seeding our full database from our (much larger) repository of script content. Part of this is for performance (preparing the test DB from fixtures is much faster than seeding all from source data), part is for reliability (ensure unit tests won't suddenly start failing when source data is updated, they may fail only when fixtures are intentionally updated/re-generated, which happens much less frequently).

Even if the video fixtures contained all the videos from the source data, it would still be useful for the latter reliability purpose (prevents unit tests from potentially breaking when we modify video-source data). I think it helps simplify the design and scope of our unit tests to have them verify only that 'our application code behaves as expected given a fixed data set', separately from the current set of ever-changing source data seeding our live application.

Separately, I do think reducing the complexity of our unit-test fixtures so that they are more (human-) readable/manageable is also a worthwhile related goal.

@islemaster
Copy link
Contributor Author

You are correct, that concern is now moot. I freaked out a little when I saw how many video keys our other fixtures referred to, but it turns out we don't need much for tests to pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants