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

Make have_file_content diffable #562

Merged

Conversation

cllns
Copy link
Contributor

@cllns cllns commented May 3, 2018

Summary

This makes the have_file_content matcher diffable in cases when it's useful, without breaking existing behavior.

Details

Custom RSpec matchers aren't diffable by default. You have to manually specify that a matcher should be diffable. This change makes this have_file_content matcher diffable. In particular, it's important for files to be diffable, since they're likely going to be several lines, and RSpec will abbreviate the expectation / actual with a darned ....

Motivation and Context

I'm a core team member for Hanami, and we used to use MiniTest for everything. We have since changed all our internal test suites to RSpec, but the generated projects still use MiniTest. I'm working on changing that, and there are a number of changes to generated files.

A big part of the Hanami ethos is to be good Ruby ecosystem citizens. We want the whole Ruby community to work well together, since it's in all of our interests. This PR will help us (it's already helped me solve my problem locally) and I'm sure it'll help others too. 🌸

How Has This Been Tested?

I did not add a test for this, since I wasn't sure how file generation worked. I did make sure that the tests did not fail (that's why I added .is_a?(String). Another approach I played with, which worked, was only showing the diff if the file was longer than one line.)

I'm happy to add a test for the diffable behavior, I just need a little direction at this point :)

Screenshots:

Before:
screen shot 2018-05-02 at 19 19 46

After:
screen shot 2018-05-02 at 19 19 02

"Better... much better"

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (cleanup of codebase without changing any existing functionality)
  • Update documentation

Checklist:

  • I've added tests for my code
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Only if expected value is a string, to ensure existing behavior stays
the same.
@cllns
Copy link
Contributor Author

cllns commented May 3, 2018

Another example I just came across, which is more significant, in that that the diff is so much longer that it doesn't it in a single terminal window.

Before:
image

After (top of screenshot):
image

@maxmeyer
Copy link
Member

maxmeyer commented May 8, 2018

As far as I know we don't have any tests for diff-functionality of our matchers. LGTM.

@xtrasimplicity
Copy link
Member

xtrasimplicity commented May 14, 2018

The AppVeyor build is failing due to ffi incompatibilities with ruby < 2.0, and byebug incompatibilities with ruby < 2.2.

Perhaps we should remove the debuggers from CI?

Edit: this is unrelated to this PR, so I'll merge this anyway.

@xtrasimplicity xtrasimplicity merged commit 7d32b8e into cucumber:master May 14, 2018
@aslakhellesoy
Copy link
Contributor

Hi @cllns,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

@olleolleolle
Copy link
Contributor

(@xtrasimplicity - removing debuggers from CI is a Good Thing.)

@xtrasimplicity
Copy link
Member

xtrasimplicity commented May 14, 2018

Agreed. I'll submit a PR when I get back to my computer, if it hasn't already been resolved in master.

Edit: There's a PR already, so I might jump on that.

@cllns
Copy link
Contributor Author

cllns commented May 14, 2018

@aslakhellesoy Thanks, I just joined!

A little disappointing to see that I've been automatically subscribed to notifications for all 35 repos in the cucumber group. I know it's not your fault, but there's no way to unsubscribe from all, so I have to click each one individually 😟 . Might just be a fault with GitHub's UI, I'm not sure, but maybe you can change the settings to not be automatically subscribed to more than just the most popular repos?

Hey there, we’re just writing to let you know that you’ve automatically started watching several repositories on GitHub.

You’ll receive notifications for all issues, pull requests, and comments that happen inside the repository. If you would like to stop watching any of these repositories, you can manage your settings here:

 https://github.com/cucumber/cucumber-ruby/subscription
 https://github.com/cucumber/cucumber-rails/subscription
 https://github.com/cucumber/aruba/subscription
 https://github.com/cucumber/cucumber-tmbundle/subscription
 https://github.com/cucumber/cucumber-cpp/subscription
 https://github.com/cucumber/cucumber-js/subscription
 https://github.com/cucumber/cucumber-jvm/subscription
 https://github.com/cucumber/cucumber-html/subscription
 https://github.com/cucumber/cucumber-lua/subscription
 https://github.com/cucumber/cucumber-jvm-deps/subscription
 https://github.com/cucumber/cucumber-eclipse/subscription
 https://github.com/cucumber/multi_test/subscription
 https://github.com/cucumber/cucumber-ruby-core/subscription
 https://github.com/cucumber/cucumber-java-skeleton/subscription
 https://github.com/cucumber/website/subscription
 https://github.com/cucumber/gherkin-ruby/subscription
 https://github.com/cucumber/gherkin-java/subscription
 https://github.com/cucumber/gherkin-javascript/subscription
 https://github.com/cucumber/gherkin-go/subscription
 https://github.com/cucumber/gherkin-dotnet/subscription
 https://github.com/cucumber/cucumber/subscription
 https://github.com/cucumber/gherkin-python/subscription
 https://github.com/cucumber/gherkin-objective-c/subscription
 https://github.com/cucumber/cucumber-tcl-wire/subscription
 https://github.com/cucumber/gherkin-perl/subscription
 https://github.com/cucumber/aruba-contrib/subscription
 https://github.com/cucumber/tag-expressions-java/subscription
 https://github.com/cucumber/tag-expressions-javascript/subscription
 https://github.com/cucumber/tag-expressions-ruby/subscription
 https://github.com/cucumber/cucumber-electron/subscription
 https://github.com/cucumber/docs.cucumber.io/subscription
 https://github.com/cucumber/aruba-getting-started/subscription
 https://github.com/cucumber/commitbit/subscription
 https://github.com/cucumber/api.cucumber.io/subscription
 https://github.com/cucumber/tag-expressions-go/subscription

You automatically watched these repositories because you’ve been given access to them.

Thanks!

@cllns cllns deleted the make-have-file-content-diffable branch May 14, 2018 21:35
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.

5 participants