Skip to content

Commit

Permalink
VSRE-414: Get TeamCity CI working (#9)
Browse files Browse the repository at this point in the history
This gets TeamCity CI working again on the version of this gem's code from #4.  We've decided that all the changes since then should be undone.  #6 has already been reverted, but #5 couldn't be done in isolation.  It was opened, after all, because the code from #4 no longer passed.

I structured this into two commits.  The first is a revert of #5.  The second is the set of test/development fixes to get TeamCity CI passing.  There are no changes to the actual gem code, so this does not need to be published as a new version.  Details:

- Update to a recent version of Ruby

- Use some Ruby `prepend` magic to normalize the `Thrift:ThinHTTPServer::RackApplication`
  response.  In most cases, a Rack app can return either an array of `[status, header, body]` or a
  `Rack::Response` that encapsulates the three.  The former is technically the only form permitted
  by the Rack specification, while the latter is what's returned by `Thrift:ThinHTTPServer`.  This
  wasn't a problem when `faraday_http_client_transport_spec.rb` was originally written, but since
  then `Rack::Lint` has been updated to allow only the array response.  I don't know why
  `Thrift:ThinHTTPServer` hasn't been updated to fix this, but this workaround is the simplest
  solution.

  See https://github.com/apache/thrift/blob/1d6a3262cf32d5063cfcb9ee09355aa1315e7f80/lib/rb/lib/thrift/server/thin_http_server.rb#L73
  for the server code and rack/rack@6ab28c2
  for the linter change.

- Regenerate the thrift files for the test with the latest Thrift version.  The old version was
  causing issues because it was expecting to receive a payload containing just the serialzed object,
  while what was actually being sent had a header that needed to be read first.  This can be seen in
  the diff for `YetiThriftTest::SimpleService::Client#recv_mutate`.

  Command used: `thrift -o spec/support --gen rb -I thrift spec/thrift/spec.thrift`

- Include `active_support` explicitly in a test that now needs it.

- Allow newer versions of `rspec` (3.x instead of 3.0.x) to work with the latest version of `rake`.

- Require `rack` v2.x for development only, since the `thin` web server used for testing does not work with `rack` 3.x

I intentionally did not try to add cloudbuild support back in.  That can be done as a follow-on
  • Loading branch information
Mark Kostick authored Jun 16, 2023
1 parent 14ac2b5 commit 1eb5246
Show file tree
Hide file tree
Showing 9 changed files with 97 additions and 47 deletions.
4 changes: 0 additions & 4 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
# yeti_thrift Changelog

## v4.0.0, May 30, 2023
- Update ruby version to 3.0.4
- Pin rake, rack, and thrift versions

## v3.0.1, October 6, 2016
- Explicitly require `Object#try` from `ActiveSupport`.
- New shared examples for testing required fields and unions.
Expand Down
24 changes: 0 additions & 24 deletions cloudbuild.yaml

This file was deleted.

1 change: 0 additions & 1 deletion lib/yeti_thrift.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
require "yeti_thrift/base64_deserializer"
require "yeti_thrift/base64_serializer"
require 'yeti_thrift/rake/thrift_gen_task'
require 'active_support'
require 'active_support/core_ext/object'

# Check field types when assigning to structs
Expand Down
1 change: 1 addition & 0 deletions spec/lib/gem_ext/thrift/struct_union/timestamp_spec.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'spec_helper'
require 'active_support'

describe Thrift::Struct_Union, 'timestamp' do

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,33 @@
require 'spec_helper'
require 'thrift/server/thin_http_server'

module ResponseNormalizer
module ClassMethods
def unpack_response(response)
if response.is_a?(Rack::Response)
[response.status, response.headers, response.body]
else
response
end
end

def successful_request(rack_request, processor, protocol_factory)
unpack_response(super)
end

def failed_request
unpack_response(super)
end
end
end


class Thrift::ThinHTTPServer::RackApplication
class << self
prepend ResponseNormalizer::ClassMethods
end
end

describe Thrift::FaradayHTTPClientTransport do

describe 'examples mocking out the actual network traffic' do
Expand Down
7 changes: 6 additions & 1 deletion spec/support/gen-rb/simple_service.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion spec/support/gen-rb/spec_constants.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

68 changes: 56 additions & 12 deletions spec/support/gen-rb/spec_types.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 6 additions & 4 deletions yeti_thrift.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,23 @@ Gem::Specification.new do |gem|
gem.test_files = gem.files.grep(%r{^(test|spec|features)/})
gem.require_paths = ["lib", "lib/yeti_thrift/gen-rb"]

gem.add_runtime_dependency 'thrift', '< 0.18'
gem.add_runtime_dependency 'thrift', '>= 0.9.1'
gem.add_runtime_dependency 'activesupport'
gem.add_runtime_dependency 'faraday'

# Include dependencies of Thrift. Not sure why they're not in
# the gemspec for thrift, but 0.9.1 doesn't seem to have a
# gemspec. Maybe a bug in that release?
gem.add_runtime_dependency 'thin'
gem.add_runtime_dependency 'rack', '2.0.1'
gem.add_runtime_dependency 'rack'

gem.add_development_dependency 'rake', '< 11.0'
gem.add_development_dependency 'rspec', '~> 3.0.0'
gem.add_development_dependency 'rake'
gem.add_development_dependency 'rspec', '~> 3.0'
gem.add_development_dependency 'yard'
gem.add_development_dependency 'redcarpet'
gem.add_development_dependency 'simplecov'
# prior to v4.0 activesupport does include tzinfo as a dependency
gem.add_development_dependency 'tzinfo'
# thin web server used for testing only works with rack 2.x
gem.add_development_dependency 'rack', '~> 2.0'
end

0 comments on commit 1eb5246

Please sign in to comment.