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

Make Provider Catalog Types Pluggable #389

Merged
merged 1 commit into from
Apr 9, 2020

Conversation

agrare
Copy link
Member

@agrare agrare commented Apr 3, 2020

@agrare agrare requested review from djberg96 and Fryguy as code owners April 3, 2020 13:15
@agrare agrare force-pushed the make_catalog_types_pluggable branch 2 times, most recently from ddb2326 to 268b148 Compare April 3, 2020 14:16
@@ -358,11 +358,11 @@ def create_factory_ems(name, region)
end
end

context 'catalog types' do
describe "#catalog types" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm totally nitpicking, and I realize that they're synonyms, but I generally use context for inner blocks, and describe for the outer block only.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's funny because in ManageIQ/manageiq#20039 (comment) I got the opposite advice :)

I tend to use describe as the top level for a method, and context if I'm testing different...well...contexts within that method

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@djberg96 I don't do describe vs context based on location (inner vs outer). I tend to recommend more like what @agrare said:

  • Use describe to describe the method under test, trying to use the actual method name if possible (e.g. ".the_method" for class methods, "#the_method" for instance methods and appending " (private)" if the method under test is private).

  • Use context to describe a particular environment in which the tests runs. (e.g. "when there are credentials defined"), and then having a before that sets up specific credentials

  • I tend to avoid creating extraneous describes and contexts unless I have more than one test. That is, if a single it is good enough, I just do that...

    # I don't really like:
    describe ".the_method" do
      it "works" do
        described_class.the_method
      end
    end
    
    # I prefer:
    it ".the_method" do
      described_class.the_method
    end

@agrare agrare force-pushed the make_catalog_types_pluggable branch from 268b148 to 2278740 Compare April 6, 2020 22:45
@miq-bot
Copy link
Member

miq-bot commented Apr 6, 2020

Checked commit agrare@2278740 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@Fryguy
Copy link
Member

Fryguy commented Apr 9, 2020

Merging on red since cross-repo is green

@Fryguy Fryguy merged commit 116fc4b into ManageIQ:master Apr 9, 2020
@agrare agrare deleted the make_catalog_types_pluggable branch April 9, 2020 17:40
@Fryguy Fryguy self-assigned this Apr 9, 2020
simaishi pushed a commit that referenced this pull request Apr 16, 2020
Make Provider Catalog Types Pluggable

(cherry picked from commit 116fc4b)
@simaishi
Copy link
Contributor

Jansa backport details:

$ git log -1
commit 0de5b37136a28f7e6e1bc55cbf9321ad5236c48b
Author: Jason Frey <[email protected]>
Date:   Thu Apr 9 13:39:52 2020 -0400

    Merge pull request #389 from agrare/make_catalog_types_pluggable

    Make Provider Catalog Types Pluggable

    (cherry picked from commit 116fc4bfadf198e2fcea20f1b9483faf7c14ff4e)

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.

5 participants