refactor: include openapi-validator as a package #191
Conversation
mvegter
changed the title
Codecov Report
@@ Coverage Diff @@
## master #191 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 33 7 -26
Lines 670 142 -528
Branches 49 14 -35
==========================================
- Hits 670 142 -528
Continue to review full report at Codecov.
|
|
@rwalle61 I believe this should do the trick |
|
Thanks @mvegter, I'm a bit busy atm but will review when I can |
There was a problem hiding this comment.
Sorry for the delay @mvegter, and thanks again for this, especially discovering the fix to make us compatible with OpenAPIResponseValidator v7!
The code looks good, see the minor comments below.
However I don't think we should merge this for several reasons that will take time to resolve. They are my fault, not yours, and I'm not asking you to fix them. I'm just being open and honest as to why they block this PR, and why I suggest you wait until I resolve them in several months.
Things blocking this PR:
- test coverage for the Jest and Chai plugins no longer reports on the
openapi-validatorcode. This is because theopenapi-validatorcode is now in a separate directory. I think it's fairly easy to merge the reports though following this for chai and Jest's equivalent for the Jest plugin) - for the same reason, mutation test coverage for the plugins no longer report on the
openapi-validatorcode. However I tried and failed to fix this in April, so we may have to sacrifice our mutation testing. I could accept this, because although mutation tests help us simplify code a bit, it is blocking several improvements to our codebase (e.g. this PR and TypeScript). - although yarn workspaces makes this
packagessetup work, we need to ensure that users of e.g. jest-openapi pull in the correct version ofopenapi-validator. Ideally we'd automatically publish all 3 packages with the same version (e.g. using lerna). But until we set that up, this is a manual process liable to error
My thought process is that merging this PR will not really benefit the user. The only thing they get is that we use an updated dev dependency (openapi-response-validator). Merging this PR will benefit us (devs) in de-duplicating the openapi-validator code, but will cost us problems 1-3.
So that's why I think that although your effort has helped us, we should park this for now. I've left comments below for reference, but I wouldn't bother implementing them.
What do you think?
| errorTransformer: ({ path, message }) => ({ | ||
| path: path.replace('response', 'object'), | ||
| message, |
There was a problem hiding this comment.
Thanks for discovering the fix to make us compatible with OpenAPIResponseValidator v7!
I would suggest keeping the details of how we use OpenAPIResponseValidator in one place (instead of changing lines 111 and 142) but doing this, assuming it works
| errorTransformer: ({ path, message }) => ({ | |
| path: path.replace('response', 'object'), | |
| message, | |
| errorTransformer: ({ path, message }) => ({ | |
| message: `${path} ${error.message.replace('response', 'object')}`, |
| @@ -1,4 +1,4 @@ | |||
| const RequestPromiseResponse = require('../../src/openapi-validator/lib/classes/RequestPromiseResponse'); | |||
| const RequestPromiseResponse = require('../../../openapi-validator/lib/classes/RequestPromiseResponse'); | |||
There was a problem hiding this comment.
This unit test file belongs in the new openapi-validator package.
However our current testing philosophy is to cover all code with e2e tests (for greatest confidence that our code works). Therefore this unit test doesn't cover any extra code. Although we may introduce unit tests eventually, until then this single extra unit test isn't valuable, so we should probably just delete it.
|
Hi @rwalle61, thanks for explaining what is blocking this pull request. At the moment we are affected (and in a way not) by your third point regarding manual releasing. The openapi-validator package.json specifies that they depend on the I'm not sure if labelling openapi-response-validator or openapi-schema-validator a dev dependency is correct as they are both regular if not the main dependencies of openapi-validator. That being said the new versions of these packags do not offer much to the end user of either jest or chai package. Moving the openapi-validator to its own package does result in dependabot picking up any changes so that is a win. Both options (merging / waiting) have their own advantages and disadvantages. Point 1 is of course annoying but not that breaking due to the openapi-validator package being tested by both the chai and jest packages. I'm not sure about point 2, I'm not familiair with mutation testing. Point 3 is not limited to this pull request as it is happening right now (see above) and will be still here after merging this pull request. However, it will more centralized as there is only a single openapi-validator package. P.S. I did do the review points so they are solved and so the coverage report link would be updated. |
|
I've just published v0.12.0 of the jest and chai plugins to npm 😄 (works for me, let me know if otherwise!) |
2 participants
No description provided.