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

Task and task spec fixes #1224

Merged
merged 4 commits into from
Jun 13, 2016

Conversation

jordan-brough
Copy link
Contributor

See individual commits:

  • Fix rake task specs - only run rake tasks once
  • Fix bug in rake task "assure_store_on_orders"
  • Change abort to raise in "assure_store_on_orders"

@jordan-brough jordan-brough added this to the 1.3.0 milestone Jun 7, 2016
@jordan-brough jordan-brough force-pushed the fix-tasks-and-task-specs branch 2 times, most recently from 925d8e6 to 5851565 Compare June 7, 2016 17:21
Our specs were running each rake task multiple times, for a couple
different reasons.  While this is great to ensure that rake tasks are
safe to run multiple times, that probably wasn't the intention. :)

First problem:  Loading the rake tasks in a `before` block meant that
we call `Rails.application.load_tasks` multiple times. And that meant
that a single task would get run multiple times. e.g.:

- spec example 1 - runs its task one time.
- spec example 2 - runs its task two times.
- spec example 3 - runs its task three times.
- ...

Second problem: `Rails.application.load_tasks` doesn't seem to work
correctly in specs.  For some reason it causes rake tasks to run
exactly twice.  So even if we avoid the previous problem mentioned
above, we'd still run each task two times.

This commit adds a "rake" shared_context that fixes these problems. It
also adds a dummy_task_spec that tests the above issues when using the
shared context.
Missing the `Spree::` prefix.
Also add some specs for the rake task.
Otherwise specs don't run properly and any configured exception
handlers may not be triggered.

And add specs.
@jhawthorn
Copy link
Contributor

This looks good to me 👍

A few additional thoughts:

  • We should probably rename assure_store_on_orders to ensure_store_on_orders
  • Does ensuring there's a store_id belong in this rake task? It seems like it could be a migration.

@jordan-brough
Copy link
Contributor Author

jordan-brough commented Jun 7, 2016

I think a migration could make sense here. Our store needs this rake task because in Solidus < 1.3 a store is only added if the Solidus-provided controllers are used to generate orders. Our app doesn't use those controllers so we have lots of orders without store ids.

@jordan-brough
Copy link
Contributor Author

Added a commit to rename assure -> ensure.

@jordan-brough
Copy link
Contributor Author

@mamhoff any thoughts on moving this to a migration?

@jhawthorn jhawthorn mentioned this pull request Jun 7, 2016
3 tasks
@jhawthorn
Copy link
Contributor

jhawthorn commented Jun 8, 2016

Thanks Jordan 👍

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.

2 participants