ruby-grape / grape

An opinionated framework for creating REST-like APIs in Ruby.
http://www.ruby-grape.org
MIT License
9.88k stars 1.22k forks source link

Declare integration tests with less repetition #2420

Closed dgutov closed 6 months ago

dgutov commented 6 months ago

Something discussed in the other PR.

It's not exactly the same behavior - each "integration" runs with the specified version of Ruby only, and doesn't run the regular test suite. But this seems easier to read anyway.

WDYT?

If running Rack 2 and 3 with all Ruby versions is really needed, I suppose integrations could be defined as a separate step (with its own matrix).

grape-bot commented 6 months ago
1 Warning
:warning: Unless you're refactoring existing code or improving documentation, please update CHANGELOG.md.
1 Message
:book: We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Here's an example of a CHANGELOG.md entry:

* [#2420](https://github.com/ruby-grape/grape/pull/2420): Declare integration tests with less repetition - [@dgutov](https://github.com/dgutov).

Generated by :no_entry_sign: Danger

dblock commented 6 months ago

This is much better. I'll merge.

I think we should be able to collapse a little further - the two tasks are very similar (bundle exec rake spec vs. bundle exec rspec spec/integration/xyz). What is being executed can probably move to the matrix itself.

dgutov commented 6 months ago

What is being executed can probably move to the matrix itself.

rake spec is kinda special: simply replacing it with rspec spec won't work because the rake task also contains the exclusions (the spec/integrations directory).

The separate step is now pretty short. We could remove it with some interpolations and long ... && ... || ... conditionals, but I don't see a way to remove the repetition of the string spec/integration/ (either in the step, or in the matrix). And with it, we repeat the conditional twice as well.

That seems hard to avoid without using an environment binding (which requires an extra step as well), or something more drastic like pre-generating the matrix in an additional process call (https://stackoverflow.com/a/65434401/615245), which is +1 step and +1 level of indirection.

So, this works, but it might not be an improvement: https://github.com/dgutov/grape/commit/af797e9333db8fad47571cb256e65eb47d181dc3

dblock commented 6 months ago

So, this works, but it might not be an improvement: dgutov@af797e9

I kinda agree. Do we need to do rake spec in both though?

dgutov commented 6 months ago

Do we need to do rake spec in both though?

No, it's rake spec for the full suite and rspec spec/integration/... for the integration tests.

dblock commented 6 months ago

Do we need to do rake spec in both though?

No, it's rake spec for the full suite and rspec spec/integration/... for the integration tests.

The rake spec runs rspec ...something eventually, what else does it do?

dgutov commented 6 months ago

what else does it do?

It adds the exclusion of integration tests:

  spec.exclude_pattern = 'spec/integration/**/*_spec.rb'
dblock commented 6 months ago

You can add --exclude-pattern on the command line too.

I don't know if it's better, but maybe.

dgutov commented 6 months ago

You can add --exclude-pattern on the command line too.

Sure. It's just that when these two invocations are different enough (we need to either call different commands in these two cases, or add different arguments anyway), it's difficult to come up with a readable way to DRY this more.

dblock commented 6 months ago

How do you feel about https://github.com/ruby-grape/grape/pull/2423?