Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test relationship.filtered #10314

Merged
merged 1 commit into from
Aug 19, 2016
Merged

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Aug 8, 2016

added tests for relationship#filtered

per #10288 (comment)

@miq-bot
Copy link
Member

miq-bot commented Aug 8, 2016

Checked commit kbrock@5284e9e with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. ⭐

vms.map(&:save!)
hosts.map(&:save!)
storages.map(&:save!)
filtered_results = Relationship.filtered(%w(Host VmOrTemplate), %w(Storage))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not...

   let!(:storages) { FactoryGirl.create_list(:relationship_storage_vmware, 1) }
   let!(:vms) { FactoryGirl.create_list(:relationship_vm_vmware, 1) }
   let!(:hosts) { FactoryGirl.create_list(:relationship_host_vmware, 1) }
   let(:filtered_results) { described_class.filtered(%w(Host VmOrTemplate), %w(Storage))

   it "scopes" do
      expect(filtered_results).to match_array(vms + hosts)
   end

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been avoiding let! - want to be obvious in the test what data exists.
the ! is so subtle and makes this hidden. It also creates objects and people refactoring don't know if it is necessary or if someone was just paying attention when adding a test.

Removing the let!(:storage) would still pass the tests, but it would not be testing that storages are not being brought back. So the test would become useless.

@Fryguy Fryguy merged commit 4c76438 into ManageIQ:master Aug 19, 2016
@Fryguy Fryguy added this to the Sprint 45 Ending Aug 22, 2016 milestone Aug 19, 2016
@Fryguy Fryguy added darga/no and removed darga/no labels Aug 19, 2016
@Fryguy
Copy link
Member

Fryguy commented Aug 19, 2016

Marked darga/yes because the previous PR #10288 is marked yes

@kbrock kbrock deleted the relationships_test branch August 20, 2016 06:45
chessbyte pushed a commit that referenced this pull request Aug 23, 2016
test relationship.filtered
(cherry picked from commit 4c76438)
@chessbyte
Copy link
Member

Darga backport details:

commit acecc1a5490973ed31efd92458776734775d2783
Author: Jason Frey <fryguy9@gmail.com>
Date:   Fri Aug 19 13:24:44 2016 -0400

    Merge pull request #10314 from kbrock/relationships_test

    test relationship.filtered
    (cherry picked from commit 4c76438ba78fbf11257927fa8a6306e324e1f1bc)

@JPrause
Copy link
Member

JPrause commented Sep 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants