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

Initial Minitest support #90

Merged
merged 6 commits into from
Mar 14, 2023
Merged

Initial Minitest support #90

merged 6 commits into from
Mar 14, 2023

Conversation

oneiros
Copy link
Contributor

@oneiros oneiros commented Feb 22, 2023

It took me a while but I think I finally have something worth discussing. This PR builds upon my proof of concept mentioned in #88, streamlines the code a little bit and adds tests.

Notable changes

  • Handling the records created from the examples run is identical between both integrations. I opted for creating a new class, ResultRecorder to move that code into so it can be shared. This makes the actual integrations a lot slimmer, which I think is very nice.
  • I need to hook into minitest in two very different ways. As such I cannot use a local outer variable as the rspec hooks do. So I created another attribute on the RSpec::OpenAPI module to collect the path_records. (A nice side-effect of the point mentioned above is that error_records has moved to the ResultRecorder class and is not needed as a global variable anymore.)

Unsolved issues

  • In contrast to my initial PoC I re-added the dependency on rspec in the .gemspec file. I had initially hoped that some kind of "soft dependency" would be possible, but there are a couple of issues with that. One is that rspec-openapi depends on and requires the rspec meta gem. rspec-rails on the other hand does not depend on that, so in many typical rails apps using rspec you will not find it in the gem bundle. I tried a couple of things but to no avail. If anyone has any ideas how one could remove the hard dependency, so minitest users will not have to install rspec, without any breaking changes for existing users, I would be thankful.
  • I tried to mirror the existing specs for the rspec integration as much as possible. And initially I was very pleasantly surprised that I was able to generate almost the same openapi files using minitest. But I was never able to get the exact same output, because with the way rspec-openapi works the output heavily depends on the order in which the tests are executed. This is a problem with minitest, as it orders tests randomly during execution. The only other option is to order them alphabetically, which sadly does not match the order in the rspec request specs either. I had to patch minitest and reorder the tests to get the exact same output, which is not ideal imho. It makes the current tests very brittle. OTOH I love that they prove that both integrations can produce the exact same output. So maybe it is OK.

Missing features

My goal was to produce something usable that can serve as a basis for discussion so there are a couple of features I intentionally did not implement. Most notably:

  • Per test-case metadata to override e.g. the description is not possible right now. Minitest has no concept of per test-case metadata, so this is rather hard to add. There are a couple of plugins for minitest that add similar functionality (e.g. tags), so it could be done, but as stated I intentionally left this out for now.
  • A custom description_builder will not work with minitest and I believe it never could because minitest has no concept of nested examples.
  • Using a Proc to construct the path will not currently work. This is actually not an intentional omission, but something I overlooked and only just noticed. I might still look into adding support for this.

Rough around the edges, but shows it can be done.
This minimizes code duplication between rspec and minitest
integrations and adds specs for minitest.

Feature-wise the minitest implementation is not yet on par
with the rspec one. Custom metadata, `description_builder`
and possibly other features are missing or will not be
possible.
Remove unused variable and re-use `path_records` to
be consistent with minitest integration.
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Rubocop found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@oneiros
Copy link
Contributor Author

oneiros commented Feb 22, 2023

I tried to mirror the existing specs for the rspec integration as much as possible. And initially I was very pleasantly surprised that I was able to generate almost the same openapi files using minitest. But I was never able to get the exact same output, because with the way rspec-openapi works the output heavily depends on the order in which the tests are executed. This is a problem with minitest, as it orders tests randomly during execution. The only other option is to order them alphabetically, which sadly does not match the order in the rspec request specs either. I had to patch minitest and reorder the tests to get the exact same output, which is not ideal imho. It makes the current tests very brittle. OTOH I love that they prove that both integrations can produce the exact same output. So maybe it is OK.

Judging from the failed checks, I still do not have a way to guarantee a stable execution order of tests ☹️

Make sure to get exact same results as request specs
in every environemnt.
@oneiros
Copy link
Contributor Author

oneiros commented Feb 27, 2023

Judging from the failed checks, I still do not have a way to guarantee a stable execution order of tests frowning_face

I made the ordering of tests explicit now, so it is 100% static. That should solve this issue.

Also, I added support for constructing the path dynamically using a Proc.

Copy link
Owner

@exoego exoego left a comment

Choose a reason for hiding this comment

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

Sorry for the late response.
LGTM 👍
I am going to fix trivial RuboCop warnings later.

@exoego exoego merged commit acc3b30 into exoego:master Mar 14, 2023
@exoego exoego added the enhancement New feature or request label Mar 14, 2023
@exoego exoego changed the title Minitest support Initial Minitest support Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants