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

Define an empty test:prepare task #2610

Open
simmerz opened this issue Jun 21, 2022 · 9 comments
Open

Define an empty test:prepare task #2610

simmerz opened this issue Jun 21, 2022 · 9 comments

Comments

@simmerz
Copy link

simmerz commented Jun 21, 2022

Gems like tailwindcss-rails and jsbundling-rails run their respective build tasks by hooking in to test:prepare which is defined as an empty task in the Test Unit railtie.

It would be helpful to define the same empty task if it's not already defined to allow those and other gems to perform their build tasks ahead of testing, rather than forcing the user to precompile assets. If the user is forced to precompile ahead of tests, then the suite needs to clobber them after running, otherwise the user becomes confused when future changes have no impact because the local development Rails app is loading the compiled assets rather than those in app/assets/build.

Would you be happy to accept a PR that adds a similar empty task as that defined here?

@pirj
Copy link
Member

pirj commented Jun 21, 2022

I wonder if it would make sense for tailwindcss-rails and alike to check if spec:prepare task exists like we do here and enhance it instead here?

What are the potential consequences of defining an empty test:prepare where it usually doesn't exist?

@simmerz
Copy link
Author

simmerz commented Jun 21, 2022

It would mean that any future gems would also need to check for additional tasks and it seems like test:prepare is trying to be a standard task placeholder.

Certainly my conversations with the rails core team have resulted in the suggestion that the placeholder task is done in rspec-rails.

From what I can see, it wouldn't have any negative impact by defining it.

@JonRowe
Copy link
Member

JonRowe commented Jun 21, 2022

We're happy to do this but seems like a workaround for a proper API rails should declare.

@pirj
Copy link
Member

pirj commented Jun 21, 2022

My main concern is that Rails would introduce an idempotency check for their non-empty definition like "define test:prepare if it isn't defined yet", and our empty definition would load earlier and would prevent the Rails one to be defined.
Are you completely certain this is not going to happen?

my conversations with the rails core team

Can you please refer to them?

@simmerz
Copy link
Author

simmerz commented Jun 21, 2022

I will absolutely do that. Just one point that I'm confused about in your last comment is that you refer to their non-empty definition, but the definition is exactly that in rails core - an empty placeholder.

Apologies if I've missed something here?

@pirj
Copy link
Member

pirj commented Jun 21, 2022

In my attempt of a thought experiment, I assumed that even though it is empty now, this may change in the future.

@simmerz
Copy link
Author

simmerz commented Jun 22, 2022

Further investigation into this would require rspec-rails to be defined in the Gemfile above anything that might want to enhance the test:prepare rake task (based on how tailwindcss-rails currently does it).

I'm more inclined to agree that this should be a task that is defined at the base Rails level at which point it is always enhanced and included right at the top of any Gemfile by convention.

I'm hoping that this finds agreement with the Rails Core team.

@pirj
Copy link
Member

pirj commented Jun 22, 2022

How does it work when a Tailwind rake task runs, the empty test:prepare Rails definition loads? They don't load Rails.
Why the same doesn't happen when you run rake spec?

@simmerz
Copy link
Author

simmerz commented Jun 22, 2022

Running rake spec loads all the rake tasks because of the Rakefile containing Rails.application.load_tasks by default.

As a result if the Rails application includes the test_unit railtie, then the test:prepare rake task is defined right at the beginning.

Therefore, if you run rake spec, with the test_unit railtie loaded, it sees that test:prepare is defined, and because tailwindcss-rails has already enhanced it, it runs the build task from there.

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

No branches or pull requests

3 participants