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

Moved to rails-based testing #58

Merged
merged 2 commits into from
May 26, 2022
Merged

Moved to rails-based testing #58

merged 2 commits into from
May 26, 2022

Conversation

exoego
Copy link
Owner

@exoego exoego commented May 26, 2022

This PR erases unit tests added in #55 and uses rails-rspec-based testing instead.
A small subset of existing OpenAPI file and test cases are added as test fixtures.

The below diff demonstrates how smart merge updates the existing fields.

❯ diff -u spec/rails/doc/smart/openapi.yaml spec/rails/doc/smart/expected.yaml
--- spec/rails/doc/smart/openapi.yaml   2022-05-27 06:51:14.000000000 +0900
+++ spec/rails/doc/smart/expected.yaml  2022-05-27 06:51:14.000000000 +0900
@@ -87,7 +87,7 @@
                     type: string
                     example: This field does not exist in rspec
               example:
-                message: This field should be updated by rspec
+                message: Unauthorized
     post:
       summary: create
       tags:
@@ -128,7 +128,7 @@
                 updated_at: '2020-07-17T00:00:00+00:00'
   "/tables/{id}":
     get:
-      summary: This filed should be updated by rspec
+      summary: show
       tags:
       - Table
       parameters:

The below diff indicates the current limitation of smart merge: it can not remove the non-existent field no_such_field.
I will add field removal in #57 later, based on this PR.

❯ diff -u spec/rails/doc/smart/openapi.yaml spec/rails/doc/openapi.yaml
--- spec/rails/doc/smart/openapi.yaml   2022-05-27 06:51:14.000000000 +0900
+++ spec/rails/doc/openapi.yaml 2022-05-27 06:51:14.000000000 +0900
(snip)
@@ -83,11 +106,8 @@
                 properties:
                   message:
                     type: string
-                  no_such_field:
-                    type: string
-                    example: This field does not exist in rspec
               example:

Context

#57 (comment)

Can you test it by only adding YAML-level tests at spec/rspec/rails_spec.rb? The only thing that must be maintained in this repository is what OpenAPI schema is generated at each rspec execution, not internal methods to implement it. If you unit-test internal implementations, it becomes harder to refactor the implementation.

So I'm not comfortable having this kind of test when you could just write a test that fails only when needed. This test could fail when it doesn't need to, for example when we rename paths_to_all_fields to something else, which users don't care about. In fact, I actually don't appreciate tests under spec/rspec/schema_merger/ because the interface of RSpec::OpenAPI::SchemaMerger.merge! isn't part of the contact of this gem. Testing the same thing by reading a YAML file generated by running rspec would be more cost-effective because it would fail only when the actual contract is changed.

end

let(:expected_path_for_smart_merge) do
File.expand_path('spec/rails/doc/smart/expected.json', repo_root)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you just convert it to a normal YAML instead of a JSON?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done in 3f9398f

Copy link
Collaborator

@k0kubun k0kubun left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@k0kubun k0kubun merged commit 52569fb into exoego:master May 26, 2022
@exoego exoego deleted the rails-rspec-based branch May 26, 2022 22:57
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.

2 participants