Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor cleaning setting up tests #1875

Merged
merged 5 commits into from Feb 12, 2023
Merged

Conversation

epergo
Copy link
Member

@epergo epergo commented Feb 12, 2023

We use minitest for Sinatra's test suite but we weren't using its rake task. I've updated the Rakefile to require and use Minitest default rake task to simplify.

Another change is to rename the helper.rb file to test_helper.rb because I think that name is used more in the community and require it directly without calling File.expand_path

@@ -1,4 +1,4 @@
require File.expand_path('helper', __dir__)
require 'test_helper'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do require_relative 'test_helper' instead?

Copy link
Member Author

@epergo epergo Feb 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used require because it's the one I've found in other projects (Rails, Pay) and because is what rubygems use when creating a new gem.

The only issue I could think of is that running tests manually from inside the test folder (or any other place different from the root path) won't work, as it won't find the file.

Do you foresee any other issue for not using require_relative?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those were the issues I was thinking about. The code before could handle those scenarios, right?

I also recall reading relative being faster.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enough arguments 😄 updated to use require_relative instead

@dentarg dentarg merged commit f10e571 into sinatra:master Feb 12, 2023
@epergo epergo deleted the ep/tests-cleanups branch May 12, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants