Much smarter merge: cleanup items that only exist in a previous result by exoego · Pull Request #57 · exoego/rspec-openapi

exoego · 2022-05-26T02:42:11Z

After this PR, some known items (below) will be removed if those exist only in a previous result.
So the removed URLs, parameters or such will be gone automatically.

paths.* URL paths
paths.*.* HTTP Methods
paths.*.*.parameters
paths.*.*.requestBody.content.application/json.example.*
paths.*.*.requestBody.content.application/json.schema.properties.*
paths.*.*.responses.*.content.application/json.example.*
paths.*.*.responses.*.content.application/json.schema.properties.*

Other items are preserved even if those exist only in a previous result.
So manually modified items can be retained.

I believe all patterns are covered by fixture-based test in smart directory.
Unit tests for internal implementations were removed not to bother refactoring in accordance with #57 (comment)

k0kubun · 2022-05-26T05:36:32Z

If we keep a first duplication in an array, the oldest example will be kept forever.
The oldest item from the removed case is always kept

While it may be the oldest in the run, It's still an example that is generated by the current run, right? If so, I don't see what's the problem with it.

This PR seems to solve too many things at once. Please at least separate a PR for __marker. If you file only other changes first, the reviewer may notice it if it indeed seems like a problem. But you're currently presenting a solution to a problem that I'm not even sure if I want to fix yet.

k0kubun · 2022-05-26T05:46:45Z

+require 'json'
+require 'rspec/openapi/schema_cleaner'
+
+RSpec.describe 'SchemaCleaner' do


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.

I understand your concerns and reasoning.
However, it is unclear for me to write tests that verify item are removed (or preserved) as expected.
IIUC, rspec-openapi read the spec file if it exist, then updates it.
To check item removal, it is required to read the old spec file with outdated items, but the file itself is updated after rails-rspec, so you or future contributors get confused about how to deal with changes in the file.
Any guidance is appreciated

A simple idea occurred to me.
I will erase unit tests and write rails-rspec-based tests

I'm okay with having a YAML fixture, like what you did at spec/rspec/schema_merger/*.json, for comparing it against an intermediate result of the schema file. If you have two YAML files for the same spec, initializing the destination path with one of the files, running rspec, and comparing the result with the other file wouldn't be too difficult.

exoego · 2022-07-25T06:33:57Z

        schema:
          type: string
        example: token
-      - name: filter[name]


Note) filter[name] was removed because it matched the pattern 3. paths.*.*.parameters

exoego · 2022-07-25T06:34:36Z

-                  no_such_field:
-                    type: string
-                    example: This field does not exist in rspec


Note) no_suche_field was removed because it matched the pattern 7. paths.*.*.responses.*.content.application/json.schema.properties.*

exoego · 2022-07-25T06:35:10Z

-                    example: This field does not exist in rspec
              example:
                message: Unauthorized
-    post:


Note) post was removed because it matched the pattern 2. paths.*.* (HTTP Methods)

exoego · 2022-07-25T06:39:14Z

+  "/no-such-path":
+    get:
+      summary: no such api
+      parameters: []
+      responses:
+        '200':
+          description: dummy
+          content:
+            application/json:
+              schema:
+                type: number
+              example:
+                - 1


Note) /no-such-path is added but does not appear in theexpected.json because it is removed because of the pattern 1) paths.* (URL paths)

exoego · 2022-07-25T06:53:42Z

+                no_such_field_request:
+                  type: boolean


Note) This is added but does not appear in theexpected.json because it is removed because of the pattern 5) paths.*.*.requestBody.content.application/json.schema.properties.*

exoego · 2022-07-25T06:54:13Z

              name: k0kubun
              description: description
              database_id: 2
+              no_such_field_request: true


Note) This is added but does not appear in theexpected.json because it is removed because of the pattern 4) paths.*.*.requestBody.content.application/json.example.*

exoego · 2022-07-25T06:54:35Z

                id: 1
                name: access
                description: logs
+                no_such_field_response: true


Note) This is added but does not appear in theexpected.json because it is removed because of the pattern 6) paths.*.*.responses.*.content.application/json.example.*

exoego · 2022-07-25T06:54:58Z

+                  no_such_field_response:
+                    type: boolean


Note) This is added but does not appear in theexpected.json because it is removed because of the pattern 7) paths.*.*.responses.*.content.application/json.schema.properties.*

exoego · 2022-07-25T06:55:33Z

@k0kubun
Rebased and simplified tests.

k0kubun · 2022-07-26T06:53:15Z

I was busy today and didn't get to review this. I'll leave comments when I can.

k0kubun · 2022-07-28T06:46:02Z

+    base
+  end
+
+  private


I think it's better to limit the scope by default, meaning that only #cleanup! should be public.

Yeah, done in d0c8c8f
Those methods were public for testing, no need to be so now.

k0kubun · 2022-07-28T07:07:12Z

Looks sensible to me. Now that you've influenced some core parts of this project's design, would you be interested in being a maintainer of this repository and merging this yourself?

exoego · 2022-07-29T06:36:50Z

Looks sensible to me. Now that you've influenced some core parts of this project's design, would you be interested in being a maintainer of this repository and merging this yourself?

I am happy to join the repo as a maintainer.

k0kubun · 2022-07-29T06:51:08Z

Thanks! I invited you to be a collaborator. I'll also make you an owner of the gem on rubygems.org, but for now, I'll wait until you accept the invitation and merge this.

k0kubun · 2022-07-29T07:11:27Z

I noticed that you might not have a RubyGems account yet. Could you create one and let me know the username? At least I couldn't find https://rubygems.org/profiles/exoego.

exoego · 2022-07-29T07:36:55Z

Created exoego in RubyGems with MFA enabled

k0kubun · 2022-07-29T17:25:56Z

Perfect 👍 I invited you to be an owner of https://rubygems.org/gems/rspec-openapi.

Release instructions

Update lib/rspec/openapi/version.rb and CHANGELOG.md
git commit those changes as "Version x.y.z"
Run rake release

Maintainer expectation

You're trusted to push changes without asking my reviews.
- No pull request or issue is needed for such changes. I prefer fewer email notifications 🙂
- If you want to leave any description, please write that down in commit messages.
You're also expected to cut a release yourself too.
- Please refer to the above instruction.

Note that at this moment my capacity is very much saturated. I get GitHub OSS notifications basically everyday and it's already beyond the point where I can handle all of them, likely at least until September. Now that you're a maintainer, please feel free to do anything that you believe is good for this project even when my response is delayed.

k0kubun reviewed May 26, 2022

View reviewed changes

This was referenced May 26, 2022

Moved to rails-based testing #58

Merged

Stabilize order of parameter objects and preserve objects with newer example #59

Merged

exoego marked this pull request as draft May 27, 2022 01:59

exoego added 3 commits July 25, 2022 15:25

Cleanup specific elements which exists only in a previous result

789d9bd

Require set explicitly for Ruby 3+

9aaa102

Update fixtures as well

8026e62

exoego commented Jul 25, 2022

View reviewed changes

Add dummy http path to be removed

e2bc5c5

exoego commented Jul 25, 2022

View reviewed changes

exoego added 2 commits July 25, 2022 15:40

Remove specs that checks implementation detail

3a9620d

Add more tests and fix bug

61b10c9

exoego commented Jul 25, 2022

View reviewed changes

exoego marked this pull request as ready for review July 25, 2022 06:55

k0kubun reviewed Jul 28, 2022

View reviewed changes

Hide internal details

d0c8c8f

exoego merged commit 2afb9b0 into exoego:master Jul 29, 2022

exoego deleted the fix branch July 29, 2022 06:54

+                  base
+                end
+                private

Conversation

exoego commented May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k0kubun commented May 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exoego Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exoego Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exoego Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exoego commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k0kubun commented Jul 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k0kubun commented Jul 28, 2022

Uh oh!

exoego commented Jul 29, 2022

Uh oh!

k0kubun commented Jul 29, 2022

Uh oh!

k0kubun commented Jul 29, 2022

Uh oh!

exoego commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k0kubun commented Jul 29, 2022

Release instructions

Maintainer expectation

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

exoego commented May 26, 2022 •

edited

Loading

exoego Jul 25, 2022 •

edited

Loading

exoego Jul 25, 2022 •

edited

Loading

exoego Jul 25, 2022 •

edited

Loading

exoego commented Jul 25, 2022 •

edited

Loading

exoego commented Jul 29, 2022 •

edited

Loading