1--- 2stage: none 3group: unassigned 4info: 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 5--- 6 7# Gotchas 8 9The purpose of this guide is to document potential "gotchas" that contributors 10might encounter or should avoid during development of GitLab CE and EE. 11 12## Do not read files from app/assets directory 13 14In GitLab 10.8 and later, Omnibus has [dropped the `app/assets` directory](https://gitlab.com/gitlab-org/omnibus-gitlab/-/merge_requests/2456), 15after asset compilation. The `ee/app/assets`, `vendor/assets` directories are dropped as well. 16 17This means that reading files from that directory fails in Omnibus-installed GitLab instances: 18 19```ruby 20file = Rails.root.join('app/assets/images/logo.svg') 21 22# This file does not exist, read will fail with: 23# Errno::ENOENT: No such file or directory @ rb_sysopen 24File.read(file) 25``` 26 27## Do not assert against the absolute value of a sequence-generated attribute 28 29Consider the following factory: 30 31```ruby 32FactoryBot.define do 33 factory :label do 34 sequence(:title) { |n| "label#{n}" } 35 end 36end 37``` 38 39Consider the following API spec: 40 41```ruby 42require 'spec_helper' 43 44RSpec.describe API::Labels do 45 it 'creates a first label' do 46 create(:label) 47 48 get api("/projects/#{project.id}/labels", user) 49 50 expect(response).to have_gitlab_http_status(:ok) 51 expect(json_response.first['name']).to eq('label1') 52 end 53 54 it 'creates a second label' do 55 create(:label) 56 57 get api("/projects/#{project.id}/labels", user) 58 59 expect(response).to have_gitlab_http_status(:ok) 60 expect(json_response.first['name']).to eq('label1') 61 end 62end 63``` 64 65When run, this spec doesn't do what we might expect: 66 67```shell 681) API::API reproduce sequence issue creates a second label 69 Failure/Error: expect(json_response.first['name']).to eq('label1') 70 71 expected: "label1" 72 got: "label2" 73 74 (compared using ==) 75``` 76 77This is because FactoryBot sequences are not reset for each example. 78 79Please remember that sequence-generated values exist only to avoid having to 80explicitly set attributes that have a uniqueness constraint when using a factory. 81 82### Solution 83 84If you assert against a sequence-generated attribute's value, you should set it 85explicitly. Also, the value you set shouldn't match the sequence pattern. 86 87For instance, using our `:label` factory, writing `create(:label, title: 'foo')` 88is ok, but `create(:label, title: 'label1')` is not. 89 90Following is the fixed API spec: 91 92```ruby 93require 'spec_helper' 94 95RSpec.describe API::Labels do 96 it 'creates a first label' do 97 create(:label, title: 'foo') 98 99 get api("/projects/#{project.id}/labels", user) 100 101 expect(response).to have_gitlab_http_status(:ok) 102 expect(json_response.first['name']).to eq('foo') 103 end 104 105 it 'creates a second label' do 106 create(:label, title: 'bar') 107 108 get api("/projects/#{project.id}/labels", user) 109 110 expect(response).to have_gitlab_http_status(:ok) 111 expect(json_response.first['name']).to eq('bar') 112 end 113end 114``` 115 116## Avoid using `expect_any_instance_of` or `allow_any_instance_of` in RSpec 117 118### Why 119 120- Because it is not isolated therefore it might be broken at times. 121- Because it doesn't work whenever the method we want to stub was defined 122 in a prepended module, which is very likely the case in EE. We could see 123 error like this: 124 125 ```plaintext 126 1.1) Failure/Error: expect_any_instance_of(ApplicationSetting).to receive_messages(messages) 127 Using `any_instance` to stub a method (elasticsearch_indexing) that has been defined on a prepended module (EE::ApplicationSetting) is not supported. 128 ``` 129 130### Alternative: `expect_next_instance_of`, `allow_next_instance_of`, `expect_next_found_instance_of` or `allow_next_found_instance_of` 131 132Instead of writing: 133 134```ruby 135# Don't do this: 136expect_any_instance_of(Project).to receive(:add_import_job) 137 138# Don't do this: 139allow_any_instance_of(Project).to receive(:add_import_job) 140``` 141 142We could write: 143 144```ruby 145# Do this: 146expect_next_instance_of(Project) do |project| 147 expect(project).to receive(:add_import_job) 148end 149 150# Do this: 151allow_next_instance_of(Project) do |project| 152 allow(project).to receive(:add_import_job) 153end 154 155# Do this: 156expect_next_found_instance_of(Project) do |project| 157 expect(project).to receive(:add_import_job) 158end 159 160# Do this: 161allow_next_found_instance_of(Project) do |project| 162 allow(project).to receive(:add_import_job) 163end 164``` 165 166Since Active Record is not calling the `.new` method on model classes to instantiate the objects, 167you 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._ 168 169If we also want to initialize the instance with some particular arguments, we 170could also pass it like: 171 172```ruby 173# Do this: 174expect_next_instance_of(MergeRequests::RefreshService, project, user) do |refresh_service| 175 expect(refresh_service).to receive(:execute).with(oldrev, newrev, ref) 176end 177``` 178 179This would expect the following: 180 181```ruby 182# Above expects: 183refresh_service = MergeRequests::RefreshService.new(project, user) 184refresh_service.execute(oldrev, newrev, ref) 185``` 186 187## Do not `rescue Exception` 188 189See ["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). 190 191This rule is [enforced automatically by 192RuboCop](https://gitlab.com/gitlab-org/gitlab-foss/blob/8-4-stable/.rubocop.yml#L911-914)._ 193 194## Do not use inline JavaScript in views 195 196Using the inline `:javascript` Haml filters comes with a 197performance overhead. Using inline JavaScript is not a good way to structure your code and should be avoided. 198 199We've [removed these two filters](https://gitlab.com/gitlab-org/gitlab/-/blob/master/config/initializers/hamlit.rb) 200in an initializer. 201 202### Further reading 203 204- Stack Overflow: [Why you should not write inline JavaScript](https://softwareengineering.stackexchange.com/questions/86589/why-should-i-avoid-inline-scripting) 205 206## Storing assets that do not require pre-compiling 207 208Assets 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. 209 210However, 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). 211 212```ruby 213support_bot = User.support_bot 214 215# accessing a file from the `app/assets` folder 216support_bot.avatar = Rails.root.join('app', 'assets', 'images', 'bot_avatars', 'support_bot.png').open 217 218support_bot.save! 219``` 220 221While the code above works in local environments, it errors out in production installations as the `app/assets` folder is not included. 222 223### Solution 224 225The alternative is the `lib/assets` folder. Use it if you need to add assets (like images) to the repository that meet the following conditions: 226 227- The assets do not need to be directly served to the user (and hence need not be pre-compiled). 228- The assets do need to be accessed via application code. 229 230In short: 231 232Use `app/assets` for storing any asset that needs to be precompiled and served to the end user. 233Use `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. 234 235MR for reference: [!37671](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/37671) 236
