BigW Consortium Gitlab

gotchas.md 5.2 KB
Newer Older
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34
# 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.

## Don't `describe` symbols

Consider the following model spec:

```ruby
require 'rails_helper'

describe User do
  describe :to_param do
    it 'converts the username to a param' do
      user = described_class.new(username: 'John Smith')

      expect(user.to_param).to eq 'john-smith'
    end
  end
end
```

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

```sh
spec/models/user_spec.rb|6 error|  Failure/Error: u = described_class.new NoMethodError: undefined method `new' for :to_param:Symbol
```

### Solution

Except for the top-level `describe` block, always provide a String argument to
`describe`.

35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123
## Don't assert against the absolute value of a sequence-generated attribute

Consider the following factory:

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

Consider the following API spec:

```ruby
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:

```sh
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 FactoryGirl 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:

```ruby
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
```

124 125 126 127 128 129 130 131 132
## Don't `rescue Exception`

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

_**Note:** This rule is [enforced automatically by
Rubocop](https://gitlab.com/gitlab-org/gitlab-ce/blob/8-4-stable/.rubocop.yml#L911-914)._

[Exception]: http://stackoverflow.com/q/10048173/223897

133
## Don't use inline JavaScript in views
134

135
Using the inline `:javascript` Haml filters comes with a
136
performance overhead. Using inline JavaScript is not a good way to structure your code and should be avoided.
137

138
_**Note:** We've [removed these two filters](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/initializers/hamlit.rb)
Robert Speicher committed
139
in an initializer._
140 141 142

### Further reading

143
- Stack Overflow: [Why you should not write inline JavaScript](http://programmers.stackexchange.com/questions/86589/why-should-i-avoid-inline-scripting)
144 145 146 147 148 149 150 151 152 153

## ID-based CSS selectors need to be a bit more specific

Normally, because HTML `id` attributes need to be unique to the page, it's
perfectly fine to write some JavaScript like the following:

```javascript
$('#js-my-selector').hide();
```

Robert Speicher committed
154 155 156
However, there's a feature of GitLab's Markdown processing that [automatically
adds anchors to header elements][ToC Processing], with the `id` attribute being
automatically generated based on the content of the header.
157 158 159 160

Unfortunately, this feature makes it possible for user-generated content to
create a header element with the same `id` attribute we're using in our
selector, potentially breaking the JavaScript behavior. A user could break the
Robert Speicher committed
161
above example with the following Markdown:
162 163 164 165 166

```markdown
## JS My Selector
```

Robert Speicher committed
167
Which gets converted to the following HTML:
168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191

```html
<h2>
  <a id="js-my-selector" class="anchor" href="#js-my-selector" aria-hidden="true"></a>
  JS My Selector
</h2>
```

[ToC Processing]: https://gitlab.com/gitlab-org/gitlab-ce/blob/8-4-stable/lib/banzai/filter/table_of_contents_filter.rb#L31-37

### Solution

The current recommended fix for this is to make our selectors slightly more
specific:

```javascript
$('div#js-my-selector').hide();
```

### Further reading

- Issue: [Merge request ToC anchor conflicts with tabs](https://gitlab.com/gitlab-org/gitlab-ce/issues/3908)
- Merge Request: [Make tab target selectors less naive](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2023)
- Merge Request: [Make cross-project reference's clipboard target less naive](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2024)