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

Only clone GitLab Shell on tests if necessary. #7823

Merged
merged 1 commit into from Sep 24, 2014

Conversation

5 participants
@cirosantilli
Contributor

cirosantilli commented Sep 22, 2014

Before this change, tmp/test got removed before every time we started running tests, and we had to clone GitLab Shell again.

Now:

  • tmp/test/gitlab-shell is not removed before tests, only the other siblings
  • it only gets cloned if the repo does not exist already
  • if the repo and the correct shell version exist, it only checks out
  • otherwise do a git fetch instead of a clone

Use case: allow to run tests without the Internet (airplane, commuting, internet problems, GitLab.com downtime, etc.)

There is only one more Internet dependency: cloning gitlab-test, but that one is harder to deal with since all branches must be synced.

Still, this change already makes testing a bit faster and walks towards that goal.

Implementation notes:

  • unless File.directory?: removed because was never used because rm_rf always removed tests, and should not be used in theory either, since if the repository is there we might want to fetch.
@TeatroIO

This comment has been minimized.

TeatroIO commented Sep 22, 2014

I've prepared a stage. Click to open.

unless File.directory?(Gitlab.config.gitlab_shell.path)
%x[rake gitlab:shell:install]
end
%x[rake gitlab:shell:install]

This comment has been minimized.

@houndci-bot

houndci-bot Sep 22, 2014

Do not use %x unless the command string contains backquotes.

@cirosantilli cirosantilli force-pushed the cirosantilli:test-fetch-shell branch from 04a71bc to 065ab3e Sep 22, 2014

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Sep 24, 2014

dzaporozhets added a commit that referenced this pull request Sep 24, 2014

Merge pull request #7823 from cirosantilli/test-fetch-shell
Only clone GitLab Shell on tests if necessary.

@dzaporozhets dzaporozhets merged commit 28b6f09 into gitlabhq:master Sep 24, 2014

1 check passed

default The build passed on Semaphore.
Details

@cirosantilli cirosantilli deleted the cirosantilli:test-fetch-shell branch Sep 24, 2014

@rspeicher

This comment has been minimized.

Member

rspeicher commented on spec/support/test_env.rb in 065ab3e Jan 29, 2015

@cirosantilli Could you explain this? This invocation adds 10+ seconds to every rspec run, even when the shell is already setup from previous invocations. Also isn't this rake task run as part of the one-time dev:setup task and shouldn't need to be run again, much less every time?

This comment has been minimized.

Contributor

cirosantilli replied Jan 29, 2015

Sorry, I don't remember very well.

As mentioned at #7823, I think before this PR gitlab shell was always removed at FileUtils.rm_r(tmp_test_path) above and recloned, and I stopped that from happening, first checking if it is already present and avoiding the clone then, which is what takes the large amount of time.

I don't remember why there was this unless check before.

Also isn't this rake task run as part of the one-time dev:setup task and shouldn't need to be run again, much less every time?

I think I didn't touch this. It was like that (run before every test suite run) to ensure that the shell is always on the right version corresponding to gitlab. E.g., if you checkout gitlab, the right shell will be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment