Skip to content

Latest commit

 

History

History
162 lines (111 loc) · 4.36 KB

gotchas.md

File metadata and controls

162 lines (111 loc) · 4.36 KB

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.

Do not assert against the absolute value of a sequence-generated attribute

Consider the following factory:

FactoryBot.define do
  factory :label do
    sequence(:title) { |n| "label#{n}" }
  end
end

Consider the following API spec:

require 'rails_helper'

describe API::Labels do
  it 'creates a first label' do
    create(:label)

    get api("/projects/#{project.id}/labels", user)

    expect(response).to have_http_status(200)
    expect(json_response.first['name']).to eq('label1')
  end

  it 'creates a second label' do
    create(:label)

    get api("/projects/#{project.id}/labels", user)

    expect(response).to have_http_status(200)
    expect(json_response.first['name']).to eq('label1')
  end
end

When run, this spec doesn't do what we might expect:

1) API::API reproduce sequence issue creates a second label
   Failure/Error: expect(json_response.first['name']).to eq('label1')

     expected: "label1"
          got: "label2"

     (compared using ==)

That's because FactoryBot sequences are not reseted for each example.

Please remember that sequence-generated values exist only to avoid having to explicitly set attributes that have a uniqueness constraint when using a factory.

Solution

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:

require 'rails_helper'

describe API::Labels do
  it 'creates a first label' do
    create(:label, title: 'foo')

    get api("/projects/#{project.id}/labels", user)

    expect(response).to have_http_status(200)
    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)

    expect(response).to have_http_status(200)
    expect(json_response.first['name']).to eq('bar')
  end
end

Avoid using allow_any_instance_of in RSpec

Why

  • 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 in a prepended module, which is very likely the case in EE. We could see error like this:

      1.1) Failure/Error: allow_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.

Alternative: expect_next_instance_of

Instead of writing:

# Don't do this:
allow_any_instance_of(Project).to receive(:add_import_job)

We could write:

# Do this:
expect_next_instance_of(Project) do |project|
  expect(project).to receive(:add_import_job)
end

If we also want to expect the instance was initialized with some particular arguments, we could also pass it to expect_next_instance_of like:

# 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:

# Above expects:
refresh_service = MergeRequests::RefreshService.new(project, user)
refresh_service.execute(oldrev, newrev, ref)

Do not rescue Exception

See "Why is it bad style to rescue Exception => e in Ruby?".

Note: This rule is enforced automatically by Rubocop.

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.

Note: We've removed these two filters in an initializer.

Further reading