Test with The Ruby Spec Suite

[glebm]

Adds The Ruby Spec Suite as a submodule and configures for running with the FasterPath monkey patches.

[glebm]

@danielpclark It runs now, please take a look!

Tests are failing because of bugs in faster_path. I've only fixed the bugs that were preventing Bundler from loading when using monkey-patched methods. Alternatively, we can avoidBundler entirely by adding another submodule with mspec, but I figured loading Bundler shouldn't be a problem once the library is stable enough.

[danielpclark]

This will be quite helpful. The Utf8Error is related to #67

[glebm]

I need to fix the regressions introduced by my "bugfixes" before this is good to merge

[danielpclark]

Do you think we can add the skip method to tests that check incompatible byte sequences instead of UTF8 compatible strings until we get an issue opened and dealt with on that?

[glebm]

Done with:

MSpec.disable_feature :encoding

[glebm]

Only 3 mspec tests are failing now:

1)
File.basename raises a TypeError if the arguments are not String types FAILED
Expected TypeError but no exception was raised (nil was returned)
/home/glebm/repos/glebm/faster_path/spec/ruby_spec/core/file/basename_spec.rb:95:in `block (2 levels) in <top (required)>'
/home/glebm/repos/glebm/faster_path/spec/ruby_spec/core/file/basename_spec.rb:5:in `<top (required)>'
/home/glebm/.rvm/gems/ruby-2.3.1@faster_path/bundler/gems/mspec-ea39a1692532/bin/mspec-run:8:in `<main>'

2)
File.basename accepts an object that has a #to_path method ERROR
TypeError: no implicit conversion of MockObject into String
/home/glebm/repos/glebm/faster_path/lib/faster_path.rb:35:in `basename'
/home/glebm/repos/glebm/faster_path/lib/faster_path.rb:35:in `basename'
/home/glebm/repos/glebm/faster_path/lib/faster_path/optional/monkeypatches.rb:7:in `basename'
/home/glebm/repos/glebm/faster_path/spec/ruby_spec/core/file/basename_spec.rb:102:in `block (2 levels) in <top (required)>'
/home/glebm/repos/glebm/faster_path/spec/ruby_spec/core/file/basename_spec.rb:5:in `<top (required)>'
/home/glebm/.rvm/gems/ruby-2.3.1@faster_path/bundler/gems/mspec-ea39a1692532/bin/mspec-run:8:in `<main>'

3)
An exception occurred during: Mock.verify_count
File.basename accepts an object that has a #to_path method FAILED
Mock 'path' expected to receive 'to_path' exactly 1 times but received it 0 times
/home/glebm/repos/glebm/faster_path/spec/ruby_spec/core/file/basename_spec.rb:5:in `<top (required)>'
/home/glebm/.rvm/gems/ruby-2.3.1@faster_path/bundler/gems/mspec-ea39a1692532/bin/mspec-run:8:in `<main>'

[danielpclark]

Since you've been working on basename directly could you also add an environment flag to the monkey patches for file? All methods that are slower than the C implementation should have this regression flag. I was going to do it but since you're working on the relevant code I'd like you to.

#monkey_patch_sledge_hammer
  # File module eval
    def slower_method
    end if ENV['WITH_REGRESSION']
  # end
#end

Same thing once we move it to Pathname

I've added this to a policy for regressions against C code on the wiki.

[danielpclark]

On the master branch only one method fails if I change FasterPath.basename to always return nil. They should all fail. I don't know why those tests are passing. Could you possibly verify why?

[glebm]

Since you've been working on basename directly could you also add an environment flag to the monkey patches for file?

I'd like to keep the changes in this PR contained to enabling RubySpec, an ENV var change should be a separate PR. I'm also not sure what the environment name should be (WITH_REGRESSION is too generic).

On the master branch only one method fails if I change FasterPath.basename to always return nil. They should all fail. I don't know why those tests are passing. Could you possibly verify why?

If it returns nil, mspec won't even run for me. Is the test perhaps failing with a core dump? Can you check again?

Rebased.

[glebm]

@danielpclark Ah, I see the env var is already set up. Added it for basename.

[glebm]

@danielpclark Any blockers to merging this?

Currently PRs are being submitted which come with tests that are an incomplete re-implementation of a part of RubySpec. If we come up with new tests for Ruby stdlib classes, we should be adding them to RubySpec instead so the entire community can benefit. These new PRs also fail RubySpec. If we merge this, the repo status will be red until the remaining tests are fixed, but the current green status is misleading.

[danielpclark]

WITH_REGRESSION is too generic

I agree with you that it is generic. But if you think about the context it will be used in it is for R&D on this project. It would be surprising though if another project did the same thing and depended upon this gem. That would be the one side effect. But I believe that if they did that and tracked down the source they would change their own env flag.

Using this gem can be done directly, or it can have the added benefit of improving Pathname. If a project wanted to access methods written in C for performance they'll be gearing for File over Pathname anyways. This project will likely move all File monkey-patches and refinements over to Pathname for added type safety, and because Pathname currently forwards these methods to File internally. With our WITH_REGRESSION flag we can better guarantee not to overwrite faster methods.

Although we may need to verify this on a system by system basis as 32bit and 64bit computers seem to have vastly different results for us. 64bit being faster for us.

[danielpclark]

I've added a new benchmark system called pbench. You can run rake pbench to see how much the method performance has improved. You will need to have benchmark code added to test/pbench/pbench_suite.rb to see it. This is brand new on the master branch.

[glebm]

Rebased

[danielpclark]

Looks like to make these tests pass we need to add #to_path and TypeError support.

[danielpclark]

Also I pulled this PR locally but the submodule's not here. How do I get it?

Never mind, I got it with: git submodule update --init