s3_source: Lazy load fog library

[Igorshp]

This speeds up all knife commands on systems where knife-ec2 plugin is
installed.

Benchmarks ( done with 'time' command):

Command	Before	After	% Improvement
knife help	8.523s	2.616s	69%
knife search "chef_environment:prod"	20.429s	15.094	26%
knife search "chef_environment:prod AND role:admin"	10.625	5.125s	51%

[Igorshp]

As far as I can see, this does not break s3 functionality, but i'm struggling to identify a correct way to test it. Any suggestions?

[btm]

Looking at other code, I'd try something like this. I'm not sure it works, so I'd test it against master too and make sure it fails.

it "loads deps all lazily" do
  expect(Chef::Knife::S3Source).not_to receive(:require)
  Chef::Knife::S3Source.new
end

[Igorshp]

Thanks @btm, i like the idea, but unforunately it doesn't work.
I believe it's because that checks for a method called 'require' on S3Source class and real 'require' is part of ruby kernel? https:/ruby/ruby/blob/ca6b174078fa15f33655be704d9409fdbc4f9929/lib/rubygems/core_ext/kernel_require.rb#L38

However:

expect(Kernel).not_to receive(:require)

also doesn't seem to work

[Igorshp]

Correct me if I'm wrong, but I'm starting to think that this is impossible to test as s3_source.rb is loaded and parsed when rspec loads.
So by the time it gets to tests, top level 'require' blocks have already been processed and do not get evaluated again.

It is also not possible to check if Fog::* is defined as it is required at the top of the rspec test and is used later for mocking.

[btm]

Yeah, might be impossible. Maybe for the second idea you move the require into the describe block somewhere and create a second describe block? I poked around the tests in core for the knife lazy loader stuff and I didn't see anything magical.

[Igorshp]

I'll try that.

I've also revised the PR. after researching how S3Source is used and how load_deps work, the original pr wouldn't have worked as run method is never called for S3Source.

Moving require to 'fog' method seems like a much simpler solution.

[willejs]

👍

[Igorshp]

@thom @btm
I've added 2 tests, but there is an interesting caveat.

• first test tries to run fog and expects name error to show that library doesn't get loaded by default
• second test, runs S3Source.fog and expects argument error to show that :fog method loads fog

If you run s3_source_deps_spec.rb on its own, it works great.

But! if it gets run as part of global tests with 'rspec', the first test fails, as fog is loaded in another spec and gets carried over.

this dependency leak essentially invalidates both of the tests.

[btm]

@Igorshp Oh yeah. I think we're totally in territory where what you're doing is a great exercise, but not required.

I could see a couple options if you want to pursue tests, but I'd be fine adding a comment about the require saying that we don't want to load fog right off.

You could run the test in shell like chef/spec/integration/client/client_spec.rb does, where you call out to ruby and make sure it doesn't raise / exit with an error.

You could add an rspec filter so the test doesn't run if Fog is already loaded (which probably means the test almost never runs, but has some documentation value).

[Igorshp]

@btm makes sense, I think that's a good solution.
I've added a spec filter and a couple of comments, it works well.

Will squash the commits once everyone is happy with the PR.

[btm]

@adamedx looks like just you and me over here. Review?

[adamedx]

👍 as long as tests all pass and we have coverage that the lazy load actually works. In general, anything we can do to speed up knife subcommands is welcome, particularly on Windows.

[btm]

@Igorshp 👍 rebase and I'll merge.

[Igorshp]

@btm all done. squashed commits, updated the commit message

[Igorshp]

Sweet, thanks @btm @adamedx