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

Declare integration tests with less repetition #2420

Merged
merged 2 commits into from
Mar 24, 2024

Conversation

dgutov
Copy link
Contributor

@dgutov dgutov commented Mar 24, 2024

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
Copy link

1 Warning
⚠️ Unless you're refactoring existing code or improving documentation, please update CHANGELOG.md.
1 Message
📖 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 🚫 Danger

@dblock
Copy link
Member

dblock commented Mar 24, 2024

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.

@dblock dblock merged commit f36011b into ruby-grape:master Mar 24, 2024
39 checks passed
@dgutov dgutov deleted the concise_integration_workflow branch March 25, 2024 00:33
@dgutov
Copy link
Contributor Author

dgutov commented Mar 25, 2024

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: dgutov@af797e9

@dblock
Copy link
Member

dblock commented Mar 25, 2024

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
Copy link
Contributor Author

dgutov commented Mar 25, 2024

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
Copy link
Member

dblock commented Mar 26, 2024

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
Copy link
Contributor Author

dgutov commented Mar 26, 2024

what else does it do?

It adds the exclusion of integration tests:

  spec.exclude_pattern = 'spec/integration/**/*_spec.rb'

@dblock
Copy link
Member

dblock commented Mar 26, 2024

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

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

@dgutov
Copy link
Contributor Author

dgutov commented Mar 26, 2024

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
Copy link
Member

dblock commented Mar 26, 2024

How do you feel about #2423?

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.

3 participants