only require ntlm if ntlm auth was requested. #187

coldnebo · 2017-07-28T14:26:02Z

coveralls · 2017-07-28T14:28:54Z

Coverage increased (+22.4%) to 96.889% when pulling 03da901 on coldnebo:ntlmerror into 2c0a725 on savonrb:master.

coveralls · 2017-07-28T15:56:59Z

Coverage decreased (-0.5%) to 74.078% when pulling 5afa0d3 on coldnebo:ntlmerror into 2c0a725 on savonrb:master.

rogerleite · 2017-07-28T17:12:10Z

LGTM 👍

coveralls · 2017-07-28T19:01:39Z

Coverage decreased (-0.5%) to 74.078% when pulling 172639d on coldnebo:ntlmerror into 2c0a725 on savonrb:master.

coldnebo · 2017-07-28T19:10:36Z

@rogerleite no problem. I tried to up the coverage, but must be missing something... a bit new to coveralls. Any advice?

rogerleite · 2017-07-28T19:35:44Z

@coldnebo sorry! No idea why coverage decreased ... 😐

coldnebo · 2017-07-31T21:23:53Z

So, coveralls says that coverage on lib/httpi/adapter/net_http.rb has fallen to 67.26%.

If I run this locally:

$ bundle exec rake ci | grep net_http.rb
|  92.98%  | lib/httpi/adapter/net_http.rb   | 114   | 8      | 41-43, 51, 63, 65, 107, 136                  |
|  66.67%  | lib/httpi/adapter/net_http.rb   | 114   | 38     | 35, 51, 53-54, 59-63, 65, 76, 98-99, 103-107, 109-1... |

So, it looks like there are two runs (~~maybe Puma?~~ actually this is because the rake ci task depends on two subtasks that run spec and spec_integration in parallel) and simplecov is being run in both and the last one out, wins... but even there... 67.26% doesn't equal either of the numbers from either of the runs.

So let's run rspec just on the one:

$ rspec spec/httpi/adapter/net_http_spec.rb | grep net_http.rb

Huh? Again, with more intensity...

$ rspec spec/httpi/adapter/net_http_spec.rb 
.........D, [2017-07-31T17:15:44.639004 #53272] DEBUG -- : Server does not support NTLM/Negotiate. Trying NTLM anyway
........

Finished in 0.04214 seconds
17 examples, 0 failures

Randomized with seed 38155


COVERAGE:  57.75% -- 503/871 lines in 20 files

showing bottom (worst) 15 of 20 files
+----------+------------------------------------------+-------+--------+-----------------------------------------------+
| coverage | file                                     | lines | missed | missing                                       |
+----------+------------------------------------------+-------+--------+-----------------------------------------------+
|  21.11%  | lib/httpi/adapter/curb.rb                | 90    | 71     | 16-17, 23-24, 27-29, 32-34, 37, 41, 43, 45... |
|  27.50%  | lib/httpi/adapter/http.rb                | 40    | 29     | 16-17, 19-20, 23-24, 32-33, 35-36, 38, 41,... |
|  27.91%  | lib/httpi/adapter/excon.rb               | 43    | 31     | 16-17, 25, 27, 29, 31, 33, 39-40, 42, 52-5... |
|  28.95%  | lib/httpi/dime.rb                        | 38    | 27     | 14, 16-18, 20-21, 24, 30, 32-36, 42-47, 52... |
|  32.65%  | lib/httpi/adapter/httpclient.rb          | 49    | 33     | 16-17, 25-26, 28, 30-31, 37, 39-41, 45-47,... |
|  34.88%  | lib/httpi/adapter/rack.rb                | 43    | 28     | 32, 37, 41, 44-46, 48, 51-52, 61-62, 65-66... |
|  39.22%  | lib/httpi/adapter/em_http.rb             | 51    | 31     | 35-36, 42, 50, 56-57, 59-60, 63-64, 67-68,... |
|  47.06%  | lib/httpi/query_builder.rb               | 17    | 9      | 12, 25-26, 33-37, 39                          |
|  50.00%  | lib/httpi/adapter/net_http_persistent.rb | 20    | 10     | 15, 19, 23-25, 29-30, 33-34, 38               |
|  55.56%  | lib/httpi/cookie_store.rb                | 9     | 4      | 18, 23-24, 30                                 |
|  60.00%  | lib/httpi/cookie.rb                      | 10    | 4      | 17, 21, 26, 31                                |
|  69.39%  | lib/httpi/auth/ssl.rb                    | 49    | 15     | 19, 42, 47-48, 52, 62-63, 67, 77-78, 82, 9... |
|  71.43%  | lib/httpi/adapter.rb                     | 35    | 10     | 26, 28-30, 34, 62-65, 67                      |
|  72.92%  | lib/httpi/response.rb                    | 48    | 13     | 32, 37, 42, 47-49, 82-83, 85, 87, 92-94       |
|  73.24%  | lib/httpi/request.rb                     | 71    | 19     | 36-38, 40-41, 46, 72, 77, 83-84, 86, 89-90... |
+----------+------------------------------------------+-------+--------+-----------------------------------------------+

net_http.rb isn't in the list?

Maybe I haven't been testing this the whole time?

Or maybe the SSL configuration is silently eating whole test cases? I noticed console errors both in my local and in Travis logs. I opened a parallel issue here: puma/puma#1381.

So confusing. Last time I worked in this codebase was before Puma was added, so I may not be understanding? Need help to close this out.

to retain coverage results. Rather than depend on the existing :spec and :spec_integration tasks, a better approach is to create a :ci Rspec task that includes all the files together. This solves the issue where Coveralls doesn't see test coverage runs for the :spec run because they are overwritten by the :spec)_integration run.

coveralls · 2017-08-01T19:15:52Z

Coverage increased (+22.6%) to 97.126% when pulling 696f650 on coldnebo:ntlmerror into 2c0a725 on savonrb:master.

rogerleite · 2017-08-01T19:20:35Z

Rakefile

-task :ci => [:spec, :spec_integration]
+RSpec::Core::RakeTask.new "ci" do |t|
+  t.pattern = "spec/{httpi,integration}/**/*_spec.rb"
+end


coldnebo · 2017-08-01T19:23:25Z

Ah, soo close. I reopened the issue with the puma team to see if they can help.

coldnebo · 2017-08-29T19:58:38Z

My investigations in the puma issue seem to have made progress and targeted the real issue having to do with Net::HTTP::Persistent timeouts under POST. Going to try checking this in and see what Travis says.

by lowering POST timeouts to 1. From Net::HTTP::Persistent history.txt: "Reducing the idle timeout is preferred over setting #retry_change_requests to true if you wish to avoid the "too many connection resets" error when POSTing data."

coldnebo · 2017-08-29T20:32:54Z

Hmmm, now 2.3.0 passes, but 2.2.4 fails and jruby/2.0.0 seem to be failing for rubygems connectivity issues? Weird.

coldnebo · 2017-08-29T20:44:00Z

Ha! Rebuilding succeeded without any code change, except for ruby 2.0.0 and jruby which still seem to be failing because ~~of bundler connection issues~~ of ruby dependency issues.

@rogerleite is this normal for this project? Let me know if I'm obsessing too much. :)

I'll run it again later and see if the bundler issue resolves itself.

coveralls · 2017-08-29T22:03:04Z

Coverage increased (+22.6%) to 97.126% when pulling c0c9ca8 on coldnebo:ntlmerror into 2c0a725 on savonrb:master.

coveralls · 2017-08-29T22:49:10Z

Coverage increased (+22.6%) to 97.126% when pulling c0c9ca8 on coldnebo:ntlmerror into 2c0a725 on savonrb:master.

some of the http clients tested) no longer supports Ruby 2.0.0 and breaks Travis CI under 2.0.0 test. In practice, not all http clients have this dependency, so I didn't want to put this restriction in the httpi.gemspec. Other options are to remove 2.0.0 from travis.yml and update the httpi.gemspec to require ruby >= 2.1.0, but this requires more discussion.

coveralls · 2017-08-30T17:47:44Z

Coverage increased (+22.6%) to 97.126% when pulling 673bb45 on coldnebo:ntlmerror into 2c0a725 on savonrb:master.

coveralls · 2017-08-30T17:47:44Z

Coverage increased (+22.6%) to 97.126% when pulling 673bb45 on coldnebo:ntlmerror into 2c0a725 on savonrb:master.

coldnebo · 2017-08-30T17:52:02Z

VICTORY!

coldnebo · 2017-10-02T22:37:39Z

@rogerleite is there anything else I need to do to get this into the next release? Thanks!

only require ntlm if ntlm auth was requested.

03da901

fixes savonrb/savon#819

fix for jruby

5afa0d3

restore coverage by explicitly testing the check ntlm code path.

172639d

coldnebo added 3 commits August 1, 2017 14:33

clean up test (forgot to unstub global state for other tests)

b462096

clean up the NTLM version test.

62c92ae

rogerleite approved these changes Aug 1, 2017

View reviewed changes

coldnebo closed this Aug 29, 2017

coldnebo reopened this Aug 29, 2017

rogerleite merged commit 7bc8ed8 into savonrb:master Oct 3, 2017

coldnebo deleted the ntlmerror branch December 1, 2017 21:46

rogerleite mentioned this pull request Jan 11, 2018

Deprecation warning for ruby 2.5.0 fixed #189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

only require ntlm if ntlm auth was requested. #187

only require ntlm if ntlm auth was requested. #187

coldnebo commented Jul 28, 2017

coveralls commented Jul 28, 2017 •

edited

Loading

coveralls commented Jul 28, 2017 •

edited

Loading

rogerleite commented Jul 28, 2017

coveralls commented Jul 28, 2017 •

edited

Loading

coldnebo commented Jul 28, 2017

rogerleite commented Jul 28, 2017

coldnebo commented Jul 31, 2017 •

edited

Loading

coveralls commented Aug 1, 2017 •

edited

Loading

rogerleite Aug 1, 2017

coldnebo commented Aug 1, 2017

coldnebo commented Aug 29, 2017

coldnebo commented Aug 29, 2017

coldnebo commented Aug 29, 2017 •

edited

Loading

coveralls commented Aug 29, 2017 •

edited

Loading

coveralls commented Aug 29, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading

coveralls commented Aug 30, 2017

coldnebo commented Aug 30, 2017

coldnebo commented Oct 2, 2017

only require ntlm if ntlm auth was requested. #187

only require ntlm if ntlm auth was requested. #187

Conversation

coldnebo commented Jul 28, 2017

coveralls commented Jul 28, 2017 • edited Loading

coveralls commented Jul 28, 2017 • edited Loading

rogerleite commented Jul 28, 2017

coveralls commented Jul 28, 2017 • edited Loading

coldnebo commented Jul 28, 2017

rogerleite commented Jul 28, 2017

coldnebo commented Jul 31, 2017 • edited Loading

coveralls commented Aug 1, 2017 • edited Loading

rogerleite Aug 1, 2017

Choose a reason for hiding this comment

coldnebo commented Aug 1, 2017

coldnebo commented Aug 29, 2017

coldnebo commented Aug 29, 2017

coldnebo commented Aug 29, 2017 • edited Loading

coveralls commented Aug 29, 2017 • edited Loading

coveralls commented Aug 29, 2017 • edited Loading

coveralls commented Aug 30, 2017 • edited Loading

coveralls commented Aug 30, 2017

coldnebo commented Aug 30, 2017

coldnebo commented Oct 2, 2017

coveralls commented Jul 28, 2017 •

edited

Loading

coveralls commented Jul 28, 2017 •

edited

Loading

coveralls commented Jul 28, 2017 •

edited

Loading

coldnebo commented Jul 31, 2017 •

edited

Loading

coveralls commented Aug 1, 2017 •

edited

Loading

coldnebo commented Aug 29, 2017 •

edited

Loading

coveralls commented Aug 29, 2017 •

edited

Loading

coveralls commented Aug 29, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading