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

tests: Fix profile_context_test NameError #3758

Merged
merged 2 commits into from Jan 31, 2019

Conversation

Projects
None yet
3 participants
@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii commented Jan 26, 2019

This closes #3721

Add missing require to Inspec::DSL#missing_method
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

Surprising location for a require; I'd want to know why (and explain with a comment).

@@ -31,6 +31,7 @@ def require_resource(options = {})
# This is called when an unknown method is encountered
# "bare" in a control file - outside of a control or describe block.
def method_missing(method_name, *arguments, &block)
require 'inspec/plugin/v2'

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Contributor

@jerryaldrichiii, can you explain why this location was chosen for the require? It would seem more typical to require in the top of lib/inspec/dsl.rb, or in lib/inspec/profile_context.rb (which is where the error appeared).

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Jan 28, 2019

Author Contributor

Great call out. I could use some words on describing the behavior succinctly. I believe the issue is because we are doing an instance_eval. The only places I could get it to work were to place the require here: https://github.com/inspec/inspec/blob/master/lib/inspec/profile_context.rb#L152 or the place you see it. I did the latter since it is closer to

registry = Inspec::Plugin::V2::Registry.instance

Placing at the top of the files like you would expect leads me down a rabbit hole that I couldn't get to the bottom of (issues like not being able to find Inspec::BaseCLI).

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Contributor

If you'd like to pair or group work on this, I'd be happy to do so; I'm familiar with the missing require for BaseCLI from the plugin/v2/plugin_types/cli type. Simple fix there.

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Jan 28, 2019

Author Contributor

Oh? Great! I'll bring it up in standup and we can get some time scheduled if the usual parking lot time doesn't cover it.

Move `require` and fix load order issue
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

@clintoncwolfe clintoncwolfe changed the title Add missing require to Inspec::DSL#missing_method tests: Fix profile_context_text NameError Jan 30, 2019

@clintoncwolfe clintoncwolfe changed the title tests: Fix profile_context_text NameError tests: Fix profile_context_test NameError Jan 30, 2019

@miah

miah approved these changes Jan 31, 2019

@clintoncwolfe clintoncwolfe merged commit 2be3063 into master Jan 31, 2019

4 checks passed

DCO This commit has a DCO Signed-off-by
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@clintoncwolfe clintoncwolfe deleted the ja/fix-profile-context-test branch Jan 31, 2019

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