debian-mirror-gitlab/doc/development/gotchas.md

248 lines
7.9 KiB
Markdown
Raw Normal View History

2021-01-29 00:20:46 +05:30
---
stage: none
group: unassigned
2021-02-22 17:27:13 +05:30
info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments
2021-01-29 00:20:46 +05:30
---
2016-06-02 11:05:42 +05:30
# Gotchas
The purpose of this guide is to document potential "gotchas" that contributors
might encounter or should avoid during development of GitLab CE and EE.
2021-01-03 14:25:43 +05:30
## Do not read files from app/assets directory
In GitLab 10.8 and later, Omnibus has [dropped the `app/assets` directory](https://gitlab.com/gitlab-org/omnibus-gitlab/-/merge_requests/2456),
after asset compilation. The `ee/app/assets`, `vendor/assets` directories are dropped as well.
2021-02-22 17:27:13 +05:30
This means that reading files from that directory fails in Omnibus-installed GitLab instances:
2021-01-03 14:25:43 +05:30
```ruby
file = Rails.root.join('app/assets/images/logo.svg')
# This file does not exist, read will fail with:
# Errno::ENOENT: No such file or directory @ rb_sysopen
File.read(file)
```
2017-08-17 22:00:37 +05:30
## Do not assert against the absolute value of a sequence-generated attribute
2016-06-02 11:05:42 +05:30
2017-08-17 22:00:37 +05:30
Consider the following factory:
2016-06-02 11:05:42 +05:30
2017-08-17 22:00:37 +05:30
```ruby
2018-03-17 18:26:18 +05:30
FactoryBot.define do
2017-08-17 22:00:37 +05:30
factory :label do
sequence(:title) { |n| "label#{n}" }
end
end
```
2016-06-02 11:05:42 +05:30
2017-08-17 22:00:37 +05:30
Consider the following API spec:
2016-06-02 11:05:42 +05:30
2017-08-17 22:00:37 +05:30
```ruby
2019-12-04 20:38:33 +05:30
require 'spec_helper'
2016-06-02 11:05:42 +05:30
2020-07-28 23:09:34 +05:30
RSpec.describe API::Labels do
2017-08-17 22:00:37 +05:30
it 'creates a first label' do
create(:label)
2016-06-02 11:05:42 +05:30
2017-08-17 22:00:37 +05:30
get api("/projects/#{project.id}/labels", user)
2016-06-02 11:05:42 +05:30
2020-03-13 15:44:24 +05:30
expect(response).to have_gitlab_http_status(:ok)
2017-08-17 22:00:37 +05:30
expect(json_response.first['name']).to eq('label1')
end
2016-06-02 11:05:42 +05:30
2017-08-17 22:00:37 +05:30
it 'creates a second label' do
create(:label)
2016-06-02 11:05:42 +05:30
2017-08-17 22:00:37 +05:30
get api("/projects/#{project.id}/labels", user)
2016-06-02 11:05:42 +05:30
2020-03-13 15:44:24 +05:30
expect(response).to have_gitlab_http_status(:ok)
2017-08-17 22:00:37 +05:30
expect(json_response.first['name']).to eq('label1')
end
end
2016-06-02 11:05:42 +05:30
```
2017-08-17 22:00:37 +05:30
When run, this spec doesn't do what we might expect:
2016-06-02 11:05:42 +05:30
2020-03-13 15:44:24 +05:30
```shell
2017-08-17 22:00:37 +05:30
1) API::API reproduce sequence issue creates a second label
Failure/Error: expect(json_response.first['name']).to eq('label1')
2016-06-02 11:05:42 +05:30
2017-08-17 22:00:37 +05:30
expected: "label1"
got: "label2"
2016-06-02 11:05:42 +05:30
2017-08-17 22:00:37 +05:30
(compared using ==)
2016-06-02 11:05:42 +05:30
```
2019-10-12 21:52:04 +05:30
This is because FactoryBot sequences are not reset for each example.
2017-08-17 22:00:37 +05:30
Please remember that sequence-generated values exist only to avoid having to
explicitly set attributes that have a uniqueness constraint when using a factory.
2016-06-02 11:05:42 +05:30
### Solution
2017-08-17 22:00:37 +05:30
If you assert against a sequence-generated attribute's value, you should set it
explicitly. Also, the value you set shouldn't match the sequence pattern.
For instance, using our `:label` factory, writing `create(:label, title: 'foo')`
is ok, but `create(:label, title: 'label1')` is not.
Following is the fixed API spec:
2016-06-02 11:05:42 +05:30
2017-08-17 22:00:37 +05:30
```ruby
2019-12-04 20:38:33 +05:30
require 'spec_helper'
2017-08-17 22:00:37 +05:30
2020-07-28 23:09:34 +05:30
RSpec.describe API::Labels do
2017-08-17 22:00:37 +05:30
it 'creates a first label' do
create(:label, title: 'foo')
get api("/projects/#{project.id}/labels", user)
2020-03-13 15:44:24 +05:30
expect(response).to have_gitlab_http_status(:ok)
2017-08-17 22:00:37 +05:30
expect(json_response.first['name']).to eq('foo')
end
it 'creates a second label' do
create(:label, title: 'bar')
get api("/projects/#{project.id}/labels", user)
2020-03-13 15:44:24 +05:30
expect(response).to have_gitlab_http_status(:ok)
2017-08-17 22:00:37 +05:30
expect(json_response.first['name']).to eq('bar')
end
end
2016-06-02 11:05:42 +05:30
```
2019-02-15 15:39:39 +05:30
## Avoid using `expect_any_instance_of` or `allow_any_instance_of` in RSpec
2018-11-08 19:23:39 +05:30
### Why
2019-03-02 22:35:43 +05:30
- Because it is not isolated therefore it might be broken at times.
- Because it doesn't work whenever the method we want to stub was defined
2018-11-08 19:23:39 +05:30
in a prepended module, which is very likely the case in EE. We could see
error like this:
2020-04-22 19:07:51 +05:30
```plaintext
2019-09-30 21:07:59 +05:30
1.1) Failure/Error: expect_any_instance_of(ApplicationSetting).to receive_messages(messages)
Using `any_instance` to stub a method (elasticsearch_indexing) that has been defined on a prepended module (EE::ApplicationSetting) is not supported.
```
2018-11-08 19:23:39 +05:30
2020-11-24 15:15:51 +05:30
### Alternative: `expect_next_instance_of`, `allow_next_instance_of`, `expect_next_found_instance_of` or `allow_next_found_instance_of`
2018-11-08 19:23:39 +05:30
Instead of writing:
```ruby
# Don't do this:
2019-02-15 15:39:39 +05:30
expect_any_instance_of(Project).to receive(:add_import_job)
2019-12-21 20:55:43 +05:30
# Don't do this:
allow_any_instance_of(Project).to receive(:add_import_job)
2018-11-08 19:23:39 +05:30
```
We could write:
```ruby
# Do this:
expect_next_instance_of(Project) do |project|
expect(project).to receive(:add_import_job)
end
2019-12-21 20:55:43 +05:30
# Do this:
allow_next_instance_of(Project) do |project|
allow(project).to receive(:add_import_job)
end
2020-11-24 15:15:51 +05:30
# Do this:
expect_next_found_instance_of(Project) do |project|
expect(project).to receive(:add_import_job)
end
# Do this:
allow_next_found_instance_of(Project) do |project|
allow(project).to receive(:add_import_job)
end
2018-11-08 19:23:39 +05:30
```
2021-01-29 00:20:46 +05:30
Since Active Record is not calling the `.new` method on model classes to instantiate the objects,
2020-11-24 15:15:51 +05:30
you should use `expect_next_found_instance_of` or `allow_next_found_instance_of` mock helpers to setup mock on objects returned by Active Record query & finder methods._
2022-05-07 20:08:51 +05:30
It is also possible to set mocks and expectations for multiple instances of the same Active Record model by using the `expect_next_found_(number)_instances_of` and `allow_next_found_(number)_instances_of` helpers, like so;
```ruby
expect_next_found_2_instances_of(Project) do |project|
expect(project).to receive(:add_import_job)
end
allow_next_found_2_instances_of(Project) do |project|
allow(project).to receive(:add_import_job)
end
```
2019-12-21 20:55:43 +05:30
If we also want to initialize the instance with some particular arguments, we
could also pass it like:
2018-11-08 19:23:39 +05:30
```ruby
# Do this:
expect_next_instance_of(MergeRequests::RefreshService, project, user) do |refresh_service|
expect(refresh_service).to receive(:execute).with(oldrev, newrev, ref)
end
```
This would expect the following:
```ruby
# Above expects:
refresh_service = MergeRequests::RefreshService.new(project, user)
refresh_service.execute(oldrev, newrev, ref)
```
2017-08-17 22:00:37 +05:30
## Do not `rescue Exception`
2019-12-21 20:55:43 +05:30
See ["Why is it bad style to `rescue Exception => e` in Ruby?"](https://stackoverflow.com/questions/10048173/why-is-it-bad-style-to-rescue-exception-e-in-ruby).
2017-08-17 22:00:37 +05:30
2021-01-29 00:20:46 +05:30
This rule is [enforced automatically by
2020-06-23 00:09:42 +05:30
RuboCop](https://gitlab.com/gitlab-org/gitlab-foss/blob/8-4-stable/.rubocop.yml#L911-914)._
2017-08-17 22:00:37 +05:30
## Do not use inline JavaScript in views
Using the inline `:javascript` Haml filters comes with a
performance overhead. Using inline JavaScript is not a good way to structure your code and should be avoided.
2021-09-04 01:27:46 +05:30
We've [removed these two filters](https://gitlab.com/gitlab-org/gitlab/-/blob/master/config/initializers/hamlit.rb)
2021-01-29 00:20:46 +05:30
in an initializer.
2017-08-17 22:00:37 +05:30
2016-06-02 11:05:42 +05:30
### Further reading
2019-12-21 20:55:43 +05:30
- Stack Overflow: [Why you should not write inline JavaScript](https://softwareengineering.stackexchange.com/questions/86589/why-should-i-avoid-inline-scripting)
2019-12-26 22:10:19 +05:30
2020-10-24 23:57:45 +05:30
## Storing assets that do not require pre-compiling
Assets that need to be served to the user are stored under the `app/assets` directory, which is later pre-compiled and placed in the `public/` directory.
However, you cannot access the content of any file from within `app/assets` from the application code, as we do not include that folder in production installations as a [space saving measure](https://gitlab.com/gitlab-org/omnibus-gitlab/-/commit/ca049f990b223f5e1e412830510a7516222810be).
```ruby
support_bot = User.support_bot
# accessing a file from the `app/assets` folder
support_bot.avatar = Rails.root.join('app', 'assets', 'images', 'bot_avatars', 'support_bot.png').open
support_bot.save!
```
While the code above works in local environments, it errors out in production installations as the `app/assets` folder is not included.
### Solution
2021-02-22 17:27:13 +05:30
The alternative is the `lib/assets` folder. Use it if you need to add assets (like images) to the repository that meet the following conditions:
2020-10-24 23:57:45 +05:30
- The assets do not need to be directly served to the user (and hence need not be pre-compiled).
- The assets do need to be accessed via application code.
In short:
Use `app/assets` for storing any asset that needs to be precompiled and served to the end user.
Use `lib/assets` for storing any asset that does not need to be served to the end user directly, but is still required to be accessed by the application code.
MR for reference: [!37671](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/37671)