diff options
Diffstat (limited to 'doc/development/gotchas.md')
-rw-r--r-- | doc/development/gotchas.md | 127 |
1 files changed, 84 insertions, 43 deletions
diff --git a/doc/development/gotchas.md b/doc/development/gotchas.md index b25ce79e89f..0f78e8238af 100644 --- a/doc/development/gotchas.md +++ b/doc/development/gotchas.md @@ -32,71 +32,112 @@ spec/models/user_spec.rb|6 error| Failure/Error: u = described_class.new NoMeth Except for the top-level `describe` block, always provide a String argument to `describe`. -## Don't `rescue Exception` +## Don't assert against the absolute value of a sequence-generated attribute -See ["Why is it bad style to `rescue Exception => e` in Ruby?"][Exception]. +Consider the following factory: -_**Note:** This rule is [enforced automatically by -Rubocop](https://gitlab.com/gitlab-org/gitlab-ce/blob/8-4-stable/.rubocop.yml#L911-914)._ +```ruby +FactoryGirl.define do + factory :label do + sequence(:title) { |n| "label#{n}" } + end +end +``` -[Exception]: http://stackoverflow.com/q/10048173/223897 +Consider the following API spec: -## Don't use inline JavaScript in views +```ruby +require 'rails_helper' -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. +describe API::Labels do + it 'creates a first label' do + create(:label) -_**Note:** We've [removed these two filters](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/initializers/hamlit.rb) -in an initializer._ - -### Further reading + get api("/projects/#{project.id}/labels", user) -- Stack Overflow: [Why you should not write inline JavaScript](http://programmers.stackexchange.com/questions/86589/why-should-i-avoid-inline-scripting) + expect(response).to have_http_status(200) + expect(json_response.first['name']).to eq('label1') + end -## ID-based CSS selectors need to be a bit more specific + it 'creates a second label' do + create(:label) -Normally, because HTML `id` attributes need to be unique to the page, it's -perfectly fine to write some JavaScript like the following: + get api("/projects/#{project.id}/labels", user) -```javascript -$('#js-my-selector').hide(); + expect(response).to have_http_status(200) + expect(json_response.first['name']).to eq('label1') + end +end ``` -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. - -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 -above example with the following Markdown: +When run, this spec doesn't do what we might expect: -```markdown -## JS My Selector -``` +```sh +1) API::API reproduce sequence issue creates a second label + Failure/Error: expect(json_response.first['name']).to eq('label1') -Which gets converted to the following HTML: + expected: "label1" + got: "label2" -```html -<h2> - <a id="js-my-selector" class="anchor" href="#js-my-selector" aria-hidden="true"></a> - JS My Selector -</h2> + (compared using ==) ``` -[ToC Processing]: https://gitlab.com/gitlab-org/gitlab-ce/blob/8-4-stable/lib/banzai/filter/table_of_contents_filter.rb#L31-37 +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 -The current recommended fix for this is to make our selectors slightly more -specific: +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: -```javascript -$('div#js-my-selector').hide(); +```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 ``` +## 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 + +## Don't 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](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/initializers/hamlit.rb) +in an initializer._ + ### 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) +- Stack Overflow: [Why you should not write inline JavaScript](http://programmers.stackexchange.com/questions/86589/why-should-i-avoid-inline-scripting) |