Skip to content
This repository has been archived by the owner on Nov 30, 2024. It is now read-only.

WIP: Merge rspec expectations #2665

Closed
wants to merge 2,383 commits into from
Closed

WIP: Merge rspec expectations #2665

wants to merge 2,383 commits into from

Conversation

p8
Copy link

@p8 p8 commented Oct 2, 2019

Proof of concept for merging the other repos into this repo while keeping all history
I had some success with the steps described here (https://thoughts.t37.net/merging-2-different-git-repositories-without-losing-your-history-de7a06bba804

git clone git@github.com:rspec/rspec-core.git
git clone git@github.com:rspec/rspec-expectations.git
cd rspec-core
git remote add rspec-expectations ../rspec-expectations
git fetch rspec-expectations
git co -b merge-rspec-expectations
git merge -S --allow-unrelated-histories rspec-expectations/master

See: #2509

alexaltair and others added 30 commits March 31, 2016 16:22
Clarify what happened in error message.
…016-04-30-for-master

Updates from rspec-dev (2016-04-30)
…016-05-04-for-master

Updates from rspec-dev (2016-05-04)
…016-05-05-for-master

Updates from rspec-dev (2016-05-05)
…016-05-07-for-master

Updates from rspec-dev (2016-05-07)
…016-05-11-for-master

Updates from rspec-dev (2016-05-11)
…-hashes

Fix include matcher so that it provides a valid diff for hashes
We only want to return a single hash from `expected`
if only one hash was provided.  If multiple hashes
were provided we do not want to merge them for the
purposes of diffing.

Also, we tend to favor `klass === object` checks
over `object.is_a?(klass)` since `object` could
be any user object that defines `is_a?` however
(or does not define it at all), whereas monkey
patching `===` on a core type like `Hash` would
be far less common and we simply don't support that.
olleolleolle and others added 21 commits April 18, 2019 22:48
…eck (rspec#1112)

* "KindOf" matchers: Rescue `NoMethodError` instead of `respond_to?` check

* "BeAnInstanceOf" matcher: Rescue `NoMethodError` inline

* BeAKindOf matcher: improve performance & error message

* BeAKindOf matcher: revert performance improvement

* BeAKindOf matcher: remove #is_a? support
CI was failing for 1.9.2 and 1.9.3. This is due to
ffi/ffi#683

Related:
- ffi/ffi#699
* Specify target Ruby version for RuboCop

Even though the aim is to provide 1.8+ support, RuboCop of the bundled
version only supports versions 2.1-2.5.

This reduces the severity of the following offence:

rspec-expectations.gemspec:30:29: C: Gemspec/RequiredRubyVersion: required_ruby_version (1.8, declared in rspec-expectations.gemspec) and TargetRubyVersion (2.5, declared in .rubocop.yml) should be equal.
  s.required_ruby_version = '>= 1.8.7'
                            ^^^^^^^^^^

* Fix Style/Encoding offence

* Set FrozenStringLiteralComment style to never

`frozen_string_literal` is not used anywhere in the code, only adding
the noise (~200 offenses) in the RuboCop check.

* Fix Layout/EmptyLines offence

* Fix Gemspec/OrderedDependencies offence

* Fix percent literal related offences

Specifically:
 - Style/PercentLiteralDelimiters
 - Style/UnneededPercentQ
 - Style/BarePercentLiterals

And find common ground for the usages to minimize the change.

* Disable Gemfile cops

They doesn't seem to cut it in a Gemfile with conditionals

* Fix Layout/SpaceAfterComma offences

* Disable Lint/AmbiguousRegexpLiteral cop for step definitions

* Fix Style/SpaceInsideBlockBraces offences

Use default styles for Style/SpaceInsideBlockBraces

* Fix Layout/TrailingBlankLines offences

* Disable Style/BlockComments cop

There's a really long explanation in the benchmark code. The diff size
is intolerable.

* Exclude Gemfile from Security/Eval check

* Disable Layout/AlignParameters locally

Even though this cop is coming from rspec-dev's default RuboCop
settings, in this repository it's kind of cumbersome to enforce either
of the styles the cop can enforce.

* Fix Style/ColonMethodCall offences

* Exclude specs from Metrics/ModuleLength check

* Fix Layout/SpaceInsideHashLiteralBraces offences

Using the default styles reduces the total change of the code required,
since with defaults the number of offences is the lowest.

                     | `space` (default) | `no_space` | `compact`
 ------------------- | ----------------- | ---------- | ---------
`no_space` (default) | 92                | 126        | 92
             `space` | 105               | 139        | 105

* Exclude specs from Metrics/BlockLength check

* Disable Style/WordArray cop

* Fix Layout/SpaceBeforeBlockBraces offences

* Fix Style/AsciiComments offence

Looks even more furious now.

* Disable Lint/HandleExceptions for benchmarks

* Disable Lint/HandleExceptions in the code

This handling is there to avoid odd messages when this example fails

* Disable Style/Semicolon

Sometimes it's more indicative.

It is very tempting to simplify

    expect {
      x = 0
      expect { x += 1; exit }.to change { x }.and cause_call_stack_jump
    }.to raise_error(/no match results, [\.\w\s]+ declare `expects_call_stack_jump\?`/)

down to

    expect {
      expect { exit }.to change { }.and cause_call_stack_jump
    }.to raise_error(/no match results, [\.\w\s]+ declare `expects_call_stack_jump\?`/)

however, maybe I just don't see how `x` facilitates the understanding
how a call stack jump is detected in a composable matcher.

* Exclude specs and benchmarks from Style/SingleLineMethods check

* Fix Style/NumericLiterals offences

* Exclude specs and benchmarks from Style/RescueModifier check

* Fix Performance/StringReplacement offence

* Fix Style/LineEndConcatenation offences

* Auto-correct Lint/UnusedBlockArgument offences

I'm not particularly sure about this commit. Probably some of the
examples are there specifically to indicate a bad code and how RSpec is
able to handle it anyway.

* Fix Style/RedundantSelf offences

* Fix Style/EvenOdd offence

* Exclude specs from Style/MultilineBlockChain check

* Fix some Lint/IneffectiveAccessModifier offences

* Fix Style/MixinGrouping offence

* Exclude specs from Style/ClassAndModuleChildren check

I tried fixing this across all specs, since it also triggers
Metrics/ModuleLength cop, but it turns out that this style is
specifically useful if a number of constants are used in the tests,
e.g. it's possible to specify
`RSpec::Expectations::MultipleExpectationsNotMetError` just by the class
name, leaving out the namespace.

* Exclude specs from Lint/AmbiguousBlockAssociation check

The following notation is quite widespread

    expect { string << "c" }.to change { string }

and it would look extremely odd being "fixed":

    expect { string << "c" }.to(change { string })

Even though sometimes it's hard to figure out what is the receiver of
the message with composable matchers, the matchers are built the way it
doesn't really matter.

* Exclude specs from Style/BracesAroundHashParameters check

This style makes total sense in specs, e.g.:

    expect({:a => 1, :b => 2}).to include(:a => 1, :b => 2)

indicates that `actual` is a hash.

* Exclude specs from Layout/SpaceInsideParens check

This style looks quite nicely:

    by( a_value_within(0.1).of(0.5) )

especially when chained with another matcher down the lines.

An example with `compose` in spec/rspec/matchers/built_in/compound_spec.rb:56:16

    expect {
      w += 1; x += 2; y += 3; z += 4
    }.to(                 combine(
      change { w }.to(1), combine(
      change { x }.to(2), combine(
      change { y }.to(3),
      change { z }.to(4) ) ) )
    )

is there for a single reason - lack of varargs in earlier, but still
supported Ruby versions.

* Exclude specific file from Style/EvalWithLocation check

The `__LINE__` would be completely useless for evals in this spec.

* Fix Layout/SpaceInsideArrayLiteralBrackets offences

One of the "offences" that went into the exclude file is there to
visually demonstrate what is in the `actual`.

* Disable Style/GlobalVars inline

I tried to keep off of in-code disabling of the cops, but since this
specific line is a hack, probably it makes sense to highlight it as
such.

* Disable Gemspec/RequiredRubyVersion

Minimum Targeted Ruby version of used RuboCop is 2.1, while even it was
dropped in rubocop/rubocop@a707c23
2.2 is past the support phase, and so will be 2.3 by the end of March
2019.

* Fix Layout/SpaceInsideReferenceBrackets offence

* Fix Style/Attr offence

* Locally disable Style/RedundantException offences

As per https://github.com/rspec/rspec-expectations/pull/1104/files#r263923497
discussion it's better to be explicit, and to suppress the cop

* Fix Layout/EmptyLinesAroundModuleBody offences

* Fix Style/NumericLiteralPrefix offences

* Fix Layout/SpaceAroundEqualsInParameterDefault offences

* Fix Layout/EmptyLinesAroundClassBody offence

* Fix Style/EmptyLiteral offences

* Fix Lint/UnusedMethodArgument offences

* Fix Performance/RedundantBlockCall offence

* Explicitly denote that the code is unreachable

It's quite obvious anyway from looking at the code, though it is a
balancing example and deserves to remain as is. However, instead of
excluding this file in .rubocop.yml, it makes sense to additionally
denote that the code is unreachable in the code itself.

* Locally disable Lint/RescueException offence

Discussion: https://github.com/rspec/rspec-expectations/pull/1104/files#r263990646

* Fix Style/MethodCallWithoutArgsParentheses offences

Also, this changes reverts fdd9ef3, for which the warnings should not
appear anymore (see https://bugs.ruby-lang.org/issues/10661).

* Fix Layout/IndentationConsistency offence

* Exclude ignored paths from RuboCop checks

Aruba creates some tmp files that don't pass the checks. Also, git
ignored bin folder (of an unknown origin, probably part of how rspec-dev
checks out and initializes repositories) contains a number of offences.

    bin/bundle:34:25: C: Style/PerlBackrefs: Avoid the use of Perl-style backrefs.
                            ^^
    bin/bundle:89:187: C: Metrics/LineLength: Line is too long. [190/186]

    tmp/aruba/rspec_expectations_spec.rb:48:4: C: Layout/TrailingBlankLines: Final newline missing.

It doesn't make sense to fix them, since those files are not meant to be
committable.

* Add RuboCop check to default rake task

* Fix typos and RuboCop reference in docs

* Add a check for RuboCop binstub

* Tell RuboCop to check all files

Previously CI check was only checking `lib` directory

* Locally disable Layout/EmptyLinesAroundArguments

It is a bug in RuboCop 0.52.1, fixed in 0.65.0
Adding a local disabling, since newer RuboCop will report an
unnecessary disabling, and it could be spotted and removed.

* Simplify regression test

The test was a bit confusing.
The point of it is that if a `matches?` is defined, and `===` is not
defined, it should not be used as an argument matcher.

rspec/rspec-expectations#1104 (comment)
- Bump dependency version
- Fix deprecations
- Limit version of transitive dependency on contracts gem on old Rubies
…ed blocks (rspec#1130)

* Add support for not swallowing expectation errors

* Add changelog for rspec#1130
…c#1132)

* Add instance variables to changes detected

* Add benchmark for large objects
…rget

https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html
https://rspec.rubystyle.guide/#implicit-block-expectations

Spec changes are due to:

    matcher.matches?(invalid_value)

doesn't work with block-only matchers, as no block is passed in, and it
fails with a message:

    1) RSpec::Matchers::BuiltIn::Change behaves like an RSpec block-only matcher uses the `ObjectFormatter` for `failure_message`
       Failure/Error: expect(message).to include("detailed inspect")
         expected "expected `@k` to have changed, but was not given a block" to include "detailed inspect"

The redundant (due to existing check in ExpectationTarget) `Proc ===
@event_proc` checks could not be removed safely as well, since
@actual_after is not initialized yet when we haven't executed the block:

     RuntimeError:
       Warnings were generated: rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/change.rb:407: warning: instance variable @actual_after not initialized

Ruby 1.8-specific workarounds:

    multiple values for a block parameter (0 for 1)

If a block expects an argument, it ought to be provided an argument in 1.8
…eing-used-with-value-expectation-target

Prevent block-only matchers from being used with value expectation target
* Notice when object implements identical `#inspect` but == returns false

In a few cases the objects under test implement identical `inspect` output but the `eq` matcher
will see a difference when doing `==`.

This can lead to misleading output like this:

```
Failures:

  1) Foo confuses users with an empty diff
     Failure/Error: Foo.something(Foo.new)

       #<Foo (class)> received :something with unexpected arguments
         expected: ("foobar")
              got: ("foobar")
       Diff:

     # ./spec/foo_spec.rb:13:in `block (2 levels) in <top (required)>'
```

This change adds a notice to help to understand this failed expectation.

Fix: rspec/rspec-support#274
@p8 p8 force-pushed the merge-rspec-expectations branch from 8625349 to 32630ab Compare October 2, 2019 19:09
@pirj
Copy link
Member

pirj commented Oct 2, 2019

WDYT of using rspec-dev or rspec as a playground for this? The goal is to have a directory structure like

README.md - common
rspec-core
 \- README.md - Core-specific readme
rspec-expectations
 \- README.md - Expectations-specific readme

It doesn't make much sense to merge README files, or, even more importantly Gemfile's from different repositories.

@p8
Copy link
Author

p8 commented Oct 2, 2019

@pirj Sure like in: https://github.com/rspec/rspec-monorepo-prototype ?
Which repo would be preferred?

@p8
Copy link
Author

p8 commented Oct 3, 2019

I've created a new pull request in rspec/rspec
rspec/rspec#48

@p8 p8 closed this Oct 3, 2019
@pirj
Copy link
Member

pirj commented Oct 3, 2019

Anything goes, that can even be a repository you own that later can be transferred to RSpec org if the experiment succeeds.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.