[FIX-1554] fixture_file_upload cannot find files

[11factory]

No description provided.

[JonRowe]

Thanks for tackling this, theres a few changes needed before we can merge this, and we need a green build.

[JonRowe]

This can't be used as it would pollute any codebase using 'rspec-rails', please refactor to not need this.

[JonRowe]

We don't support Rails < 3 so this isn't needed.

[JonRowe]

s/describe/RSpec.describe/ our test suite doesn't allow monkey patching

[JonRowe]

Do we need to store the value if we can't set it? I think it'd be cleaner if we just implemented this in the main mix in and delegated directly to this with the resolved file.

[11factory]

yes because ActionDispatch::TestProcess is expecting class to respond to fixture_path
And I have to override setter method because of Rails 3 implementation of fixture_file_upload.

[11factory]

@JonRowe CI seems to have some random failures / between 2 builds with exact same code differents environments are failing

[JonRowe]

yes because ActionDispatch::TestProcess is expecting class to respond to fixture_path

Thats fine we can implement fixture_path

And I have to override setter method because of Rails 3 implementation of fixture_file_upload.

Also fine, including the file does that hence why delegation works at all..

To be clear I think we should remove the delegation.

[11factory]

I'm not sure to get your point - ActionController::TestCase is a class, I don't see a really better way to inject it the fixture_path

[JonRowe]

module RSpec
  module Rails
    # @private
    module FixtureFileUploadSupport

      def self.included(other)
        if defined?(ActionDispatch::TestProcess)
          other.include ActionDispatch::TestProcess
        end
      end

      def fixture_path
        @fixture_path ||= calculate_path
      end

      def fixture_path=(value)
        # Rails 3.0..3.1 are using ActionController::TestCase class to resolve fixture_path
        # see https://apidock.com/rails/v3.0.0/ActionDispatch/TestProcess/fixture_file_upload
        if ActionController::TestCase.respond_to?(:fixture_path)
          ActionController::TestCase.fixture_path = value
        end
        @fixture_path = value
      end

      private

      def calculate_fixture_path
        resolved_fixture_path = (fixture_path || RSpec.configuration.fixture_path || '')
        self.fixture_path = File.join(resolved_fixture_path, '')
      end
    end
  end
end

[JonRowe]

When FixtureFileUploadSupport is included it will define the relevant helpers.

[11factory]

fixture_path has to be defined as a class method / attribute given how rails resolves it: self.class.respond_to?(:fixture_path) && self.class.fixture_path.
And directly including ActionDispatch::TestProcess will come with other methods not related like cookies, session, ... , I prefer to keep isolated from that class as much as possible.

[JonRowe]

Oh that wasn't clear, in that case you need to do the definition inside another module and extend it:

module RSpec
  module Rails
    # @private
    module FixtureFileUploadSupport

      def self.included(other)
        if defined?(ActionDispatch::TestProcess)
          other.include ActionDispatch::TestProcess
        end
        other.extend FixturePathAttribute
      end

      module FixturePathAttribute

        def fixture_path
         @fixture_path ||= calculate_path
        end

        def fixture_path=(value)
          # Rails 3.0..3.1 are using ActionController::TestCase class to resolve fixture_path
          # see https://apidock.com/rails/v3.0.0/ActionDispatch/TestProcess/fixture_file_upload
          if ActionController::TestCase.respond_to?(:fixture_path)
            ActionController::TestCase.fixture_path = value
          end
          @fixture_path = value
        end

        private

        def calculate_fixture_path
          resolved_fixture_path = (fixture_path || RSpec.configuration.fixture_path || '')
          self.fixture_path = File.join(resolved_fixture_path, '')
        end
      end
    end
  end
end

[11factory]

@JonRowe ping

[11factory]

I did the changes as suggested, but then still including unecessary methods to initial caller, it's your call.

[11factory]

@JonRowe finally, suggested implementation was not working as expected.
I reverted the code to my first proposal, I let you see if you want to take it as it is or just drop it.

[JonRowe]

Thanks, I'll refactor it a bit before release

[bmulholland]

This seems to have caused a regression: #1921