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

add --create flag #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add --create flag #21

wants to merge 1 commit into from

Conversation

tradiff
Copy link

@tradiff tradiff commented Jul 8, 2022

Improve the initial setup experience by allowing the user to run bundle exec turbo_tests --create to create the databases. Also add documentation regarding this new onboarding process. This may help with #12 being less of a hurdle for users coming over from parallel_tests, as well as brand new users.

Copy link
Collaborator

@ilyazub ilyazub left a comment

Choose a reason for hiding this comment

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

@travistx Thank you for your contribution and for explaining the DX problem!

What if we'll just update the documentation like in my comment below?

Comment on lines +13 to +18
def self.create(count)
ENV["PARALLEL_TEST_FIRST_IS_1"] = "true"
command = ["bundle", "exec", "rake", "db:create", "RAILS_ENV=#{ParallelTests::Tasks.rails_env}"]
args = { count: count.to_s }
ParallelTests::Tasks.run_in_parallel(command, args)
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if it should be added to the Runner.

I understand it'd be nicer to reduce the number of steps on the user side. But in this case, it seems, that wrapping two commands in a bash/rake command fits better.

Comment on lines +64 to +68
Create test databases

```bash
$ bundle exec turbo_tests --create
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Create test databases before running tests

  1. Start TEST_ENV_NUMBER from 1
export PARALLEL_TEST_FIRST_IS_1=true
  1. Create databases
bundle exec rake db:create RAILS_ENV=test

Copy link
Author

Choose a reason for hiding this comment

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

That doesn't create the necessary test databases. That will just create a single "_test" database, not one database per cpu.

Here is your suggestion:

❯ export PARALLEL_TEST_FIRST_IS_1=true
❯ bundle exec rake db:create RAILS_ENV=test
Database 'myapp_test' already exists

Here is my suggestion with this PR:

❯ bundle exec turbo_tests --create
Database 'myapp_test5' already exists
Database 'myapp_test9' already exists
Database 'myapp_test10' already exists
Database 'myapp_test1' already exists
Database 'myapp_test3' already exists
Database 'myapp_test7' already exists
Database 'myapp_test8' already exists
Database 'myapp_test6' already exists
Database 'myapp_test2' already exists
Database 'myapp_test4' already exists

The way to do this without any changes to turbo_tests is first install the parallel_test gem as a direct dependency, and then run this:

❯ export PARALLEL_TEST_FIRST_IS_1=true
❯ bundle exec rake parallel:create
Database 'myapp_test3' already exists
Database 'myapp_test1' already exists
Database 'myapp_test9' already exists
Database 'myapp_test2' already exists
Database 'myapp_test8' already exists
Database 'myapp_test5' already exists
Database 'myapp_test6' already exists
Database 'myapp_test10' already exists
Database 'myapp_test4' already exists
Database 'myapp_test7' already exists

But the problem with this is it it requires having the parallel_tests gem installed as a direct dependency. And this is a direct dependency that is only used for the purpose of this one-time command. With my PR, the user is no longer required to install parallel_tests as a direct dependency. They can just install turbo_tests which acts as their gateway to parallel_tests.

@bf4
Copy link

bf4 commented Feb 29, 2024

seems reasonable to document this even if not done automatically?

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

3 participants