[webui][ci] Initial tests for model Comment #2247

eduardoj · 2016-10-20T13:19:47Z

No description provided.

Ana06 · 2016-10-21T08:16:31Z

I think that the commit message is not very clear. If what you meant is that you haven't tested all the method what about something like

[ci] Create test for Comment model

Not all the methods of the model have been tested.

Also, does the [webui] tag make sense here?

Ana06 · 2016-10-21T08:20:30Z

You could also test the belong_to and has_many relations. For example for the first one:

it { should belong_to(:bs_request).inverse_of(:comments) }

Ana06 · 2016-10-21T08:35:23Z

src/api/spec/models/comment_spec.rb

+  let!(:comment) { create(:comment) }
+
+  describe "A comment" do
+    it { expect(comment).to be_valid }


Does it make sense to check if an object that has already been created is valid?

I think it should be like:

it 'has a valid factory' do expect(build(:comment)).to be_valid end

Ana06 · 2016-10-21T08:38:20Z

src/api/spec/models/comment_spec.rb

+  end
+
+  describe "blank_or_destroy" do
+    context "without children" do


Why hasn't you tested the with children case?

Ana06 · 2016-10-21T08:42:03Z

src/api/spec/models/comment_spec.rb

+  end
+
+  describe "to_xml" do
+    context "without parents" do


shouldn't it be without parent? in singular I mean.

Do we really need a context for that? I would directly test the with parent case. We could also keep both cases, with and without parent.

Ana06 · 2016-10-21T08:45:51Z

src/api/spec/models/comment_spec.rb

+      }
+
+      context "returns xml" do
+        it { expect(builder.class.name).to eq 'Nokogiri::XML::Builder' }


why do we want to test that?

Ana06 · 2016-10-21T08:50:00Z

src/api/spec/models/comment_spec.rb

+
+  describe "to_xml" do
+    context "without parents" do
+      let(:builder) {


this let is strange. Why not just

let(:builder) { Nokogiri::XML::Builder.new } let(:comment_to_xml) { comment.to_xml(builder) }

And then testing that the content of the comment is what is expected

Ana06 · 2016-10-21T08:50:21Z

src/api/spec/models/comment_spec.rb

+        it { expect(builder.class.name).to eq 'Nokogiri::XML::Builder' }
+
+        context "comment element" do
+          let(:comment_element) { builder.doc.css('comment') }


why do you need that?

Ana06 · 2016-10-21T16:04:17Z

src/api/spec/factories/comment.rb

+FactoryGirl.define do
+  factory :comment do
+    body { Faker::Lorem.paragraph }
+    type { Faker::Lorem.word }


You shouldn't use a random word as a type as it is reserved for storing the the inheritance class. Because of the same reason and taking into account that it is not possible to create a commentwithout type (because of the presence validation) it doesn't make sense to test comment. You should test the comment children. I think that for some function it is enough with testing it for one child (for example comment_package), although for some of them it may be needed to test it for more.

Also, I added a validation for that in #2252 and this should fail in the tests after it is merged (it already fails outside the tests).

Ana06 · 2016-10-21T16:39:33Z

src/api/spec/models/comment_spec.rb

+require "rails_helper"
+
+RSpec.describe Comment do
+  let!(:comment) { create(:comment) }


You don't need the !

In case that you need the object to be created (like happens in the blank_or_destroy test) you can do

describe "blank_or_destroy" do before do comment end

That way you don't create it for all tests.

hennevogel · 2016-11-02T15:43:09Z

@eduardoj what's the status of this one?

eduardoj · 2016-11-03T08:42:22Z

@hennevogel: @Ana06, @bgeuken and me have been working on it, and still are. We are facing problems when we execute the whole test suite, but not when we execute the test alone. We have created a branch with some changes: https://github.com/eduardoj/open-build-service/tree/test-comment

Ana06 · 2016-11-03T09:05:35Z

Yes, how the database is cleaned after every feature is wrong as it is deleting the users in the seeds and this needs to be solved before writing this test. 😉

hennevogel · 2016-11-04T11:10:05Z

So I guess you should rebase now :-)

Ana06 · 2016-11-04T09:06:47Z

src/api/spec/models/comment_spec.rb

+require "rails_helper"
+
+RSpec.shared_examples "a comment" do
+  let(:comment_type) { described_class.name.underscore }


what about calling it comment_factory? It is the name of the factory what we get here described_class.name.underscore, so it may be more clear.

Ana06 · 2016-11-04T10:14:51Z

src/api/spec/models/comment_spec.rb

+  end
+
+  describe "to_xml" do
+    context "without parent" do


As I said:

Do we really need a context for that? I would directly test the with parent case. We could also keep both cases, with and without parent.

I would keep either both cases or the with parent case that is more general, but not only the without parent .case

Ana06 · 2016-11-04T12:42:59Z

src/api/spec/models/comment_spec.rb

+        create(comment_type, parent: comment)
+      end
+
+      it 'should be destroyed' do


This should be it 'shouldn't be destroyed' do

Ana06 · 2016-11-04T12:44:12Z

src/api/spec/models/comment_spec.rb

+
+      it 'should be destroyed' do
+        expect { comment.blank_or_destroy }.to_not change { Comment.count }
+


why is it this blank line here?

Ana06 · 2016-11-04T12:47:02Z

src/api/spec/models/comment_spec.rb

+        expect { comment.blank_or_destroy }.to_not change { Comment.count }
+
+        expect(comment.body).to eq 'This comment has been deleted'
+        expect(comment.user).to eq User.find_nobody!


I would test check the user login instead of the whole User to ensure that the find_nobody method is working properly:

expect(comment.user.login).to eq '_nobody_'

Ana06 · 2016-11-04T13:01:52Z

src/api/spec/models/comment_spec.rb

+
+  describe "to_xml" do
+    context "without parent" do
+      let(:builder) {


This is still strange. Why not doing

let(:builder) { builder = Nokogiri::XML::Builder.new }

and then

before do comment.to_xml(builder) end

that should be enough

Ana06 · 2016-11-04T13:04:16Z

src/api/spec/models/comment_spec.rb

+          let(:comment_element) { builder.doc.css('comment') }
+
+          it "comment attributes and content" do
+            expect(comment_element.attribute('id').value).to match(/^\d+$/)


why don't you check the exactly value?

Ana06 · 2016-11-04T16:16:16Z

src/api/spec/models/comment_spec.rb

+    it { is_expected.to belong_to(:package).inverse_of(:comments) }
+    it { is_expected.to belong_to(:user).inverse_of(:comments) }
+
+    it { is_expected.to have_many(:children) }


why not

it { is_expected.to have_many(:children).dependent(:destroy).class_name('Comment').with_foreign_key('parent_id') }

http://www.rubydoc.info/github/thoughtbot/shoulda-matchers/Shoulda%2FMatchers%2FActiveRecord%3Ahave_many

Ana06 · 2016-11-04T16:23:27Z

Please delete the last commit, shouldnt be there

We are catching the nobody user in the User#find_nobody method and it is only used here. It is confusing and the Rails query cache will cache this too, so we will only do one more query per thread. Also, the openSUSE#2247 PR is failing because of this way of caching users and how the database is cleaned, and it was difficult to get to know what the error was.

Ana06 · 2016-11-04T17:11:59Z

Squash 8b5002a into 28d1740 😉

Ana06 · 2016-11-07T08:32:27Z

src/api/spec/models/comment_spec.rb

+
+          it "comment attributes and content" do
+            expect(comment_element.attribute('id').value).to eq(comment.id.to_s)
+            expect(comment_element.attribute('when').value).to match(/^\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d UTC$/)


why don't you check the exactly value?

Ana06 · 2016-11-07T08:32:33Z

src/api/spec/models/comment_spec.rb

+          it "comment attributes and content" do
+            expect(comment_element.attribute('id').value).to eq(comment.id.to_s)
+            expect(comment_element.attribute('when').value).to match(/^\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d UTC$/)
+            expect(comment_element.attribute('who').value).to match(/^user_\d+$/)


why don't you check the exactly value?

Ana06 · 2016-11-07T08:32:45Z

src/api/spec/models/comment_spec.rb

+            expect(comment_element.attribute('when').value).to match(/^\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d UTC$/)
+            expect(comment_element.attribute('who').value).to match(/^user_\d+$/)
+
+            expect(comment_element.text).to match(/^#<Nokogiri::XML::Builder::NodeBuilder:0x\h+>$/)


why don't you check the exactly value?

Ana06 · 2016-11-07T08:34:19Z

src/api/spec/models/comment_spec.rb

+      end
+    end
+
+    context "with parent" do


This tests is exactly the same that the one above but with one more attribute. Why have you left it empty? I would prefer to have the with parent case as it is "more general".

Ana06 · 2016-11-10T09:50:55Z

src/api/spec/models/comment_spec.rb

+
+  describe "to_xml" do
+    context "without parent" do
+      let(:builder) { builder = Nokogiri::XML::Builder.new }


This variable is defined several times, so you could put it up and define it only once.

Ana06 · 2016-11-10T10:05:10Z

src/api/app/models/comment.rb

@@ -60,8 +60,8 @@ def to_xml(builder)
    attrs = { who: user, when: created_at, id: id }
    attrs[:parent] = parent_id if parent_id

-    builder.comment_(attrs) do
-      builder.text(body)
+    builder.comment_(attrs) do |xml|


Why did you change that?

Ana06 · 2016-11-10T10:07:11Z

src/api/spec/models/comment_spec.rb

+          let(:comment_element) { builder.doc.css('comment') }
+
+          it "comment attributes and content" do
+            expect(comment_element.attribute('id').value).to eq(comment.id.to_s)


Isn't it possible to do that using the result of the .to_xml ?

Ana06 · 2016-11-11T16:59:24Z

src/api/spec/models/comment_spec.rb

+
+      context "returns xml" do
+        context "comment element" do
+          let(:comment_element) { builder.doc.css('comment') }


let(:comment_element) { builder.doc.css('comment') } is also several time, please move it up so you don't need to have it twice.

Ana06 · 2016-11-14T10:17:06Z

src/api/spec/models/comment_spec.rb

+    it { is_expected.to validate_presence_of(:user) }
+  end
+
+  describe "to_xml" do


I am not pretty sure if there is a better way to test this function. @bgeuken can you take a look?

I am also not sure. Let's not invest more time into this without changing the to_xml implenentation for comments.

ChrisBr · 2016-11-14T10:41:58Z

src/api/spec/models/comment_spec.rb

+  end
+
+  describe "to_xml" do
+    let(:builder) { builder = Nokogiri::XML::Builder.new }


The assignment is not necessary, is it?

ChrisBr · 2016-11-14T10:43:49Z

src/api/spec/models/comment_spec.rb

+        create(comment_factory, parent: comment)
+      end
+
+      it 'shouldn\'t be destroyed' do


I would prefer double quotes instead of escaping the single quote!
"shouldn't be destroyed"

ChrisBr · 2016-11-14T10:46:01Z

@eduardoj Is this still dependent on #2297 or ready to merge?

Ana06 · 2016-11-14T10:49:08Z

@ChrisBr it doesn't depends on #2297

Not all the methods of the model have been tested.

bgeuken · 2016-11-14T17:02:09Z

Looks good. Let's ship this!

eduardoj added the Frontend Things related to the OBS RoR app label Oct 20, 2016

eduardoj force-pushed the tests_for_model_comment branch 5 times, most recently from 12bfff6 to 559d3ae Compare October 21, 2016 07:33

eduardoj force-pushed the tests_for_model_comment branch from 559d3ae to b061095 Compare October 21, 2016 08:18

Ana06 suggested changes Oct 21, 2016

View reviewed changes

eduardoj force-pushed the tests_for_model_comment branch from b061095 to 0bbb9d9 Compare October 21, 2016 11:51

Ana06 mentioned this pull request Oct 21, 2016

[api][webui] Add validation for Comment type #2252

Merged

Ana06 suggested changes Oct 21, 2016

View reviewed changes

eduardoj force-pushed the tests_for_model_comment branch 3 times, most recently from a3ebf71 to 6cdbe43 Compare October 28, 2016 08:40

eduardoj force-pushed the tests_for_model_comment branch from 6cdbe43 to 736c920 Compare November 3, 2016 10:37

eduardoj added the DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR label Nov 3, 2016

Ana06 mentioned this pull request Nov 3, 2016

[api] Get rid of the nobody user thread caching #2294

Merged

Ana06 suggested changes Nov 4, 2016

View reviewed changes

eduardoj force-pushed the tests_for_model_comment branch from 493686a to e6a44a0 Compare November 4, 2016 16:28

eduardoj force-pushed the tests_for_model_comment branch from e6a44a0 to 70d3176 Compare November 4, 2016 16:39

eduardoj force-pushed the tests_for_model_comment branch from 70d3176 to 8b5002a Compare November 4, 2016 16:57

eduardoj removed the DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR label Nov 4, 2016

Ana06 suggested changes Nov 7, 2016

View reviewed changes

eduardoj force-pushed the tests_for_model_comment branch 3 times, most recently from e42d28b to 458eef8 Compare November 9, 2016 09:55

Ana06 suggested changes Nov 10, 2016

View reviewed changes

eduardoj force-pushed the tests_for_model_comment branch from 458eef8 to 3387429 Compare November 11, 2016 10:51

Ana06 suggested changes Nov 11, 2016

View reviewed changes

eduardoj force-pushed the tests_for_model_comment branch from 3387429 to b17d1b5 Compare November 11, 2016 17:14

Ana06 approved these changes Nov 14, 2016

View reviewed changes

ChrisBr suggested changes Nov 14, 2016

View reviewed changes

[ci] Create test for Comment model

26454b9

Not all the methods of the model have been tested.

eduardoj force-pushed the tests_for_model_comment branch from b17d1b5 to 26454b9 Compare November 14, 2016 14:22

bgeuken merged commit d906c10 into openSUSE:master Nov 14, 2016

eduardoj deleted the tests_for_model_comment branch March 23, 2017 11:54


		it 'should be destroyed' do
		expect { comment.blank_or_destroy }.to_not change { Comment.count }

[webui][ci] Initial tests for model Comment #2247

[webui][ci] Initial tests for model Comment #2247

Conversation

eduardoj commented Oct 20, 2016

Ana06 commented Oct 21, 2016

Ana06 commented Oct 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ana06 Oct 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hennevogel commented Nov 2, 2016

eduardoj commented Nov 3, 2016

Ana06 commented Nov 3, 2016 • edited

hennevogel commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ana06 commented Nov 4, 2016

Ana06 commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisBr commented Nov 14, 2016

Ana06 commented Nov 14, 2016

bgeuken commented Nov 14, 2016

Ana06 commented Oct 21, 2016 •

edited

Ana06 Oct 21, 2016 •

edited

Ana06 commented Nov 3, 2016 •

edited