Closed nejclovrencic closed 11 months ago
@nejclovrencic thanks for the pull request. Some tests break. Can you check it, please?
@daniloab tests should pass now, audit test was failing because of vulnerability in qs
. I have updated express to the latest minor and body-parser to the latest major to fix the vulnerability. It's used only as dev dependency, so shouldn't have any effect in prod.
@daniloab do you have any ETA when we can get this merged? Can we release a new minor version, including this fix?
@daniloab do you have any ETA when we can get this merged? Can we release a new minor version, including this fix?
I would like to have a test plan for every merge change. Can we have someone to test your changes? https://dev.to/woovi/test-plan-driven-development-56a2
@daniloab I have added the test plans in the PR description. I have tested the change, and will ask someone from my team to test this as well.
@daniloab I have added the test plans in the PR description. I have tested the change, and will ask someone from my team to test this as well.
Sure, I agree. I mean tiny steps:
1. install the lib
2. try to build with this schema
3. see the error/ 3. see the succes
I can verify that it works as expected after following the test plan in the PR description. I've created a repo for testing - https://github.com/shekharnwagh/swagger-jsdoc-fix-test.
I can verify that it works as expected after following the test plan in the PR description. I've created a repo for testing - shekharnwagh/swagger-jsdoc-fix-test.
this is really cool, thanks @shekharnwagh
is this error expected with the pull request?
@daniloabyes, this is expected. There is a broken symlink (src/app.js links to itself), so the library with the PR throws the error. If you change failOnError: false
in index.js
the code will run without any problems. You can observe similar behaviour even with cat
, because the link is broken on purpose, for testing this PR.
> cat src/app.js
cat: src/app.js: Too many levels of symbolic links
> ls -la
total 8
drwxr-xr-x 4 nejclovrencic staff 128 Jun 5 10:16 .
drwxr-xr-x 10 nejclovrencic staff 320 Jun 5 10:18 ..
lrwxr-xr-x 1 nejclovrencic staff 6 Jun 5 10:16 app.js -> app.js
-rw-r--r-- 1 nejclovrencic staff 552 Jun 5 10:16 router.js
is this error expected with the pull request?
@daniloab I added that symlink to create a scenario where we can test the new failOnError
flag.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
do you have any ETA when we can get this merged? Can we release a new minor version, including this fix?
@daniloab Would it be possible to merge and release this.
is this error expected with the pull request?
@daniloab I added that symlink to create a scenario where we can test the new
failOnError
flag.
can you check the audit flow, please? https://github.com/Surnet/swagger-jsdoc/actions/runs/5763184661/job/15624477586
When running
build
, the for loop goes through each file and callsextractAnnotations
and executes parsing logic. Any of this logic could throw, for examplereadFileSync
insideextractAnnotations
could throw. Currently, this fails the whole process.This PR wraps everything inside the for loop in a try catch, and throws based on
failOnError
options.This PR also replaced
eslint-loader
because it is deprecated andnpm install
on master branch currently fails, aseslint-loader
does not support eslint v8.Test plans
1
failOnError: false
2