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

Fix diff for compound when transforming actual [onto main] #1440

Merged

Conversation

henrahmagix
Copy link

@henrahmagix henrahmagix commented Dec 17, 2023

fixes #1317
fixes #1406
resolves #1319

I cherry-picked the commits from #1319 onto 3-12-maintenance, they applied without conflicts, then I ran the scripts called in the test CI action and they all passed. I pushed this to 3-12-maintenance...henrahmagix:fix-compound-redefined-actual-diff-on-3-12 just for good measure =)

I did the same on main, bundle installed fine, but I wasn't able to run any of the tests locally because I kept getting the following error:

$ script/run_build
============= Starting binstub check ===============
Checking required binstubs
============= Ending binstub check ===============
============= Starting specs ===============
/path/to/rspec-expectations/bin/rspec
Could not find compatible versions

Because every version of rspec-core depends on rspec-support ~> 3.12.0
  and every version of rspec-expectations depends on rspec-support = 3.13.0.pre,
  every version of rspec-core is incompatible with rspec-expectations >= 0.
So, because Gemfile depends on rspec-expectations >= 0
  and Gemfile depends on rspec-core >= 0,
  version solving has failed.

So I hope the tests pass similarly when run in CI because I can't confirm locally unfortunately ☺️

@pirj
Copy link
Member

pirj commented Dec 19, 2023

There’s something fishy going on with the 1.8.7 build and the implicit expectation detection.
Also activesupport in cukes

============= Starting cukes ===============
[105](https://github.com/rspec/rspec-expectations/actions/runs/7239633754/job/19722746571?pr=1440#step:8:106)
/home/runner/work/rspec-expectations/rspec-expectations/bin/cucumber
[106](https://github.com/rspec/rspec-expectations/actions/runs/7239633754/job/19722746571?pr=1440#step:8:107)
/home/runner/work/rspec-expectations/rspec-expectations/bundle/ruby/2.7.0/gems/activesupport-7.1.2/lib/active_support/core_ext/time/conversions.rb:62:in `<class:Time>': undefined method `deprecator' for ActiveSupport:Module (NoMethodError)

.to output(/baz/).to_stdout
.and output(/qux/).to_stderr
}
.to fail_including(
Copy link
Member

@JonRowe JonRowe Dec 20, 2023

Choose a reason for hiding this comment

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

I think this is what is breaking the built, it seems antiquated [because it is] but 1.8.7 required:

thing.
to(

over:

thing
.to(

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thank you!
With this, the 4.0 pr is redundant.
Jon, do you mind merging thus and handling ci for diff-lcs separately?

@pirj pirj force-pushed the fix-compound-redefined-actual-diff-on-main branch from 0c6ae44 to 7e13447 Compare December 28, 2023 19:38
pirj and others added 4 commits December 28, 2023 23:02
Previously, we were passing the untransformed actual to the differ.
Now, we take it from the matchers.

fixes rspec#1317
@pirj pirj force-pushed the fix-compound-redefined-actual-diff-on-main branch from 7e13447 to 9b44765 Compare December 28, 2023 20:03
@pirj pirj merged commit d0bb212 into rspec:main Dec 28, 2023
30 checks passed
@pirj
Copy link
Member

pirj commented Dec 28, 2023

Thank you for pushing this to completion, @henrahmagix !

@henrahmagix henrahmagix deleted the fix-compound-redefined-actual-diff-on-main branch December 28, 2023 20:27
@henrahmagix
Copy link
Author

Thanks to you and Jon for fixing and finishing it! 🙌

JonRowe pushed a commit that referenced this pull request Feb 4, 2024
…al-diff-on-main

Fix diff for compound when transforming actual
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants