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

Remove level_source fixture #38194

Merged
merged 4 commits into from
Dec 16, 2020
Merged

Remove level_source fixture #38194

merged 4 commits into from
Dec 16, 2020

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Dec 10, 2020

This one-and-a-half megabyte yaml file has been causing some issues when loading level fixtures, and looks to be set to cause even more issues on Rails 6.

Specifically, when trying to load fixtures as a part of rake db:test:prepare, we get the error Mysql2::Error: Commands out of sync; you can't run this command now. It seems to be related to an issue with Mysql's max_allowed_packet configuration, We previously tried to bypass this by increasing the setting on the client (#38123) but it looks like to actually fix it we also need to increase the setting on the server, which is of course a bit more complicated.

Instead, I'd simply like to do away with this enormous fixture. Looks like we have a total of three tests which are relying on it; two of which could simply be using FactoryBot instead to create the data they need, the third of which seems to just be testing that fixtures are getting loaded and so (unless I'm fundamentally misunderstanding the purpose of the test) is not testing anything that we actually care about.

Follow-up work

We've made a couple of attempts to adjust the max_allowed_packet value; once we've merged this, we can go through and undo those since this should render them unnecessary.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@Hamms Hamms marked this pull request as ready for review December 11, 2020 20:23
@Hamms Hamms changed the title remove level source fixture Remove level_source fixture Dec 11, 2020
Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Whoa, nice work Elijah! We are 100% on board with moving off test fixtures. This one looks like it was easier to remove than I feared. I just ran into the max packet problem recently on my local environment. This seems like it will help a lot!

@Hamms
Copy link
Contributor Author

Hamms commented Dec 11, 2020

Haha, yeah I'm not gonna lie I was pretty shocked to see a green checkmark on this after fixing just those three tests

@Hamms Hamms requested a review from a team as a code owner December 14, 2020 19:12
@Hamms Hamms changed the base branch from staging-next to staging December 14, 2020 19:12
@Hamms Hamms merged commit c940510 into staging Dec 16, 2020
@Hamms Hamms deleted the remove-level-source-fixture branch December 16, 2020 22:21
Hamms added a commit that referenced this pull request Dec 16, 2020
Follow-up to #38194; our attempts to fix fixture loading by increasing max_allowed_packet weren't really working, so instead we "fixed" the issue by just removing the fixture that was causing those problems.
@Hamms Hamms added the Rails Upgrade All work related to upgrading the version of Ruby on Rails we use. label Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rails Upgrade All work related to upgrading the version of Ruby on Rails we use.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants