Migrate test suite to rspec

[ghost]

It seems time to move on from shindo to rspec since active development on shindo has waned. This issue is for a discussion on the topic. At the moment I'm trying to get feedback on using webmock/rspec in place of firing up servers wherever possible for the sake of fewer moving parts and a leaner test suite.

Note that we can still it seems we could still use all of the rackups, etc. See https://github.com/bblimke/webmock

Then again, the way it all works in the test suite is pretty simple.

[ghost]

After much thought, I think real servers ( current method ) is still the way to go.

[geemus]

Yeah, I could see arguments on both sides, but given the nature of things (low-level http) I would be a bit anxious that mocks might lose/miss things that real servers would catch. In particular it might end up testing specifics of the mock implementation more so than actual usage. That said, I suppose it might well be perfectly fine also (hard to know). We already have a good track record with (and patterns for) servers though, so just sticking with that seems safest.

[ghost]

@geemus Whole heartily agree, I was trying to be more open minded about mocks, but I do. I completely agree. Mocks are dangerous IMHO and should be used only if need be.

That aside, why is unicorn specifically tested? Was is just for the chunked responses? I believe we can do that with rack now. Also, looking into to see how others are handling this in Ruby, I think Capybara has the right idea: https://github.com/jnicklas/capybara/blob/master/spec/server_spec.rb

[geemus]

I don't recall why unicorn specifically. I don't think it was chunked, I think it had to do with some of the tests around multi-threading?

[ghost]

@geemus with_unicorn is called in basic and proxy tests. In basic, all the test are within the context of streaming.

[geemus]

It looks like it was originally added for unix socket tests, and later adapted for streaming, see: 54a8511

[ghost]

@geemus Right, for some reason I thought Webrick was capable of streaming and so forth at this point, I was wrong : ) with_unicorn stays.

[ghost]

@geemus Right, getting going on this now. How do you want to do this PR? Specifically, several small PRs or one big PR? Doing big PRs always feels wrong. Also, if we do several small PRs then we can either flip the switch after all the tests are migrated or we can run shindo and then rspec.

Have a sandbox here which is currently passing

No problems, except for with_server... Sometimes in travis the test fails for jruby because the process either fails to start or exits prematurely, this is intermittent. I suppose I could try putting in a sleep(5) if it's jruby, see if that works.

Notes:

• TODO: Add Excon::Test::Server. An abstraction for starting and stopping servers, etc. Need to re-work tests to utilize before and after hooks to handle starting and shutting down servers (i.e., server.start or server.stop)

• history

  Also, let me know if you want to change anything, that is dir/file layout and so forth.

[geemus]

Yeah, similar to you my inclination would be several smaller PRs if we can manage it. Just gives more leeway for course correction and/or error recovery should we need it.

Not sure on the jruby failure, unfortunately.

[ghost]

@geemus Ok first PR will merely introduce rspec into the mix. As for the jruby failure, I think this will be solved when I re-work everything to use before/after hooks. with_foo do {} doesn't play well with the way rspec executes each example. So, I'm going to abstract the nitty gritty details of starting and stopping servers into a class which can be utilized within a test suite. The caller will have to start the server in before(:all) and stop it in after(:all).

[geemus]

Sounds like a plan, thanks!

On Mon, Jul 11, 2016 at 5:36 PM, Bryan Paxton notifications@github.com
wrote:

@geemus https://github.com/geemus Ok first PR will merely introduce
rspec into the mix. As for the jruby failure, I think this will be solved
when I re-work everything to use before/after hooks. with_foo do {} doesn't
play well with the way rspec executes each example. So, I'm going to
abstract the nitty gritty details of starting and stopping servers into a
class which can be utilized within a test suite. The caller will have to
start the server in before(:all) and stop it in after(:all).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#575 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAAQKnsJoKxhVbk7XE3bpR92ZoQecbOIks5qUsVUgaJpZM4I0_OO
.

[ghost]

Note: To follow along https://github.com/starbelly/excon/tree/rspec

[geemus]

Awesome, thanks again!

On Tue, Jul 12, 2016 at 3:47 PM, Bryan Paxton notifications@github.com
wrote:

Note: To follow along https://github.com/starbelly/excon/tree/rspec

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#575 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAAQKsXSSgwMQdr26xTKceJN-5FVKXISks5qU_1NgaJpZM4I0_OO
.

[ghost]

@geemus Helios.

Finding some time to wrap this up. Had a question about context in tests/bad_tests.rb. Specifically, the first two tests are within the context of expecting the server to cause an EOFError, yet the tests within this context expect a response. It's only the last test in a new context with a different server that actually expects an EOF error. Just wanted clarification on all that.

Of course, we can just move everything over and worry about adjusting context and so forth later, but I figured this was the the best time provided the contexts of these PRs and move to rspec, no pun intended

Here's basically what it looks like after all is said (sans tweaking the context, etc.):

describe 'Excon bad server interaction' do
  context('bad server: causes EOFError') do
    before do
      @server = Excon::Test::Server.new(:exec => exec_path('bad.rb'))
      @server.start
    end
    after do
      @server.stop
    end

    context('with no content length and no chunking') do
      context('without a block') do
        it 'should respond with hello' do
          connection = Excon.new('http://127.0.0.1:9292')

          body = connection.request(:method => :get, :path => '/eof/no_content_length_and_no_chunking').body
          expect(body).to eq 'hello'
        end
      end
    end
  end
end

[geemus]

Yeah, it's weird. I think the expectation is that the bad server is just closing the connection, which really does cause an EOF, we just rescue it within the context of the read, so it just returns nil (instead of raising all the way back out), because it is actually an expected behavior in this case. See: https://github.com/excon/excon/blob/master/lib/excon/socket.rb#L205. So the context is not very helpfully worded, but arguably it's not totally inaccurate either. Does that clarify?

[ghost]

@geemus Most certainly does.

[ghost]

@geemus Ignore that last comment about basic_tests... : )

[geemus]

No worries.

[ghost]

Can we close this in favor of #575 or vice versa? Trying to get number of open issues down.

[geemus]

@starbelly this is #575, so I'm guessing you meant a different issue. I'm fine consolidating, but I wasn't sure which other issue you meant. Could you clarify? Thanks!

[ghost]

LOL Yeah... :) I mean either #311 or this one. 'bout to start sending PRs for this again. Company I'm with is going to start using gitlab, chef, etc... I said, "Well then we need to donating some of my time to excon then" :)

[geemus]

Got it, closed #311. Thanks!

[ghost]

Hello @geemus :) Been almost a year eh? :) I've got some time now though to finish this up...

After a lot of time passing, reflection, and new experiences.... I see an opportunity to not only finish this up but make it much more simple. Coming back to read the tests after a year I find that some are very confusing. The tests are good, I believe, but unless you have a solid understanding and a good bit of experience with rspec, the end-to-end tests are just mind boggling at first, IMHO. And that's coming from the guy who re-wrote them with a focus on making them readable. The output is very readable, but reading the actual tests, not so good....

I think end-to-end and/or integration tests should be done via cucumber. Rspec is great for unit tests and some of of the integration, but I think gherkin and cucumber really fit the bill better than no other in regards to this area of testing.

IMHO this lowers the barrier to entry if you want to get involved in writing tests via executable specifications and solid suite of re-usable steps (ala aruba where the goal is to not have to write anything but gherkin).

Anwho... Let me know your thoughts. If you agree I'll proceed. Otherwise I'll just finish up the rspec tests.

P.S. I've had a lot of great experiences with gherkin recently :) For integration, e2e, and even unit tests. Though, it's contextual... I'm finding sometimes gherkin is the tool for the job, sometimes a plain ole unit test is still the way to go.

[geemus]

Hey, welcome back. (sorry for my delay in responding, travel last week has me behind on things).

Simplicity and more shared stuff seems good (as well as making it more accessible).

I'm more on the fence about cucumber though, call-by-regex has always seemed uncomfortable to me. Similarly, the times I've tried to use it (which are admittedly limited), it seemed to get in my way as often as not. Maybe that is just the initial phase, after which things flow more smoothly.

Anyway, I'm certainly willing to discuss it further. And I would definitely agree that having clearer testing with more re-usablility would be a win, I just remain somewhat skeptical of cucumber in particular for this purpose. I guess I'm also not that familiar with how aruba does things, is it just using cucumber also, or is it just a similar approach?

Thanks for reaching back out and starting this discussion!

[ghost]

Let me get in an example put together and pushed up... Cucumber is weird at first, and you're right, it's the initial setup. But once you've got a few done, it quickly becomes a fast paced "thing". Also, to re-iterate, I don't think cucumber or gherkin should be used for ALL the tests, just some. I think we should have unit tests wherever possible for methods that are data in/out and integration tests for the rest. Preferably we should always have more unit tests vs integration.

Anyway, example coming soon.

[geemus]

Sounds good, I agree that it should be easier to discuss in the context of a particular example. Thanks!

[stale]

This issue has been automatically marked stale due to inactivity. It will be closed if no further activity occurs. Thank you for your contributions.

[geemus]

Pinning, as this remains the overall plan, just time hasn't allowed...