Created attachment 168708 [details] [review]
Bug 37302: Set test to failed if swagger-cli missing

This will ensure we set the test to failed if swagger-cli is found to be missing.

We also should update ktd/jenkins to ensure swagger-cli is present for the CI test runs so we catch the real errors.

Created attachment 168710 [details] [review]
Bug 37302: Add swagger-cli to devDependancies

This patch adds swagger-cli 4.0.4+ to the devDependancies section of
package.json. This should ensure it gets installed when appropriate

Totally agree, and then we should remove it from ktd's Dockerfiles

126 # Install some tool
127 RUN yarn global add gulp-cli swagger-cli

(In reply to Jonathan Druart from comment #4)
> Totally agree, and then we should remove it from ktd's Dockerfiles
> 
> 126 # Install some tool
> 127 RUN yarn global add gulp-cli swagger-cli

What is the reasoning here?

(In reply to Tomás Cohen Arazi from comment #5)
> (In reply to Jonathan Druart from comment #4)
> > Totally agree, and then we should remove it from ktd's Dockerfiles
> > 
> > 126 # Install some tool
> > 127 RUN yarn global add gulp-cli swagger-cli
> 
> What is the reasoning here?

Hum? What do you mean?

We should remove swagger-cli from ktd's Dockerfiles and have it in koha's package.json (what Martin's patch does).

(In reply to Tomás Cohen Arazi from comment #5)
> (In reply to Jonathan Druart from comment #4)
> > Totally agree, and then we should remove it from ktd's Dockerfiles
> > 
> > 126 # Install some tool
> > 127 RUN yarn global add gulp-cli swagger-cli
> 
> What is the reasoning here?

Ok, just read your conversation on mattermost and got it.

The relevant bit on the docker file is:

# Embed /kohadevbox/node_modules
RUN cd /kohadevbox \
    && wget -q https://gitlab.com/koha-community/Koha/-/raw/main/package.json?inline=false -O package.json \
    && wget -q https://gitlab.com/koha-community/Koha/-/raw/main/yarn.lock?inline=false -O yarn.lock \
    && yarn install --modules-folder /kohadevbox/node_modules \
    && mv /root/.cache/Cypress /kohadevbox && chown -R 1000 /kohadevbox/Cypress \
    && rm -f package.json yarn.lock

That's where and how the node_modules dir is built. If you wanted to reproduce locally.

The point here is that if we need additional libraries/tools for the test suite to run successfully then they must be part of Koha, not injected by ktd.

I had a go at testing, but this is not my wheelhouse and I don't think I've done this correctly.

Does this mean that:
1. an actual test is failing, and the cause of this needs fixing, 
   but
2. that is OK as this bug was fixing the reason the failed test was not showing?

Testing notes (using KTD):
1. Run prove xt/api.t -v
2. Tests pass (before patches) - but note  "ok 1 # skip Skipping tests, swagger-cli missing" [1]
3. Apply the first patch and re-run: tests should fail [2]
4. Apply the second patch.
5. IGNORE: Shutdown KTD and the start it up again (I tried a yarn build and a reset_all first, but got same results as in [2].
6. Ran yarn install the restart_all [3]
   Note this warning: warning swagger-cli > @apidevtools/swagger-cli@4.0.4: This package has been abandoned. Please switch to using the actively maintained @redocly/cli
7. I expected the tests to now pass, but they fail [4]

[1] Tests pass before any patches are applied:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

prove xt/api.t -v
xt/api.t .. 
1..4
ok 1 - GET /api/v1/
ok 2
# Subtest: The spec passes the swagger-cli validation
    1..1
    ok 1 # skip Skipping tests, swagger-cli missing
ok 3 - The spec passes the swagger-cli validation
# Subtest: tags tests
    1..1
    ok 1 - No tag errors in the spec
ok 4 - tags tests
ok
All tests successful.
Files=1, Tests=4,  4 wallclock secs ( 0.02 usr  0.00 sys +  3.54 cusr  0.17 csys =  3.73 CPU)
Result: PASS

[2] Tests fail after first patch is applied:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

prove xt/api.t -v
xt/api.t .. 
1..4
ok 1 - GET /api/v1/
ok 2
# Subtest: The spec passes the swagger-cli validation
    1..1
    not ok 1 - Test skipped, swagger-cli missing

    #   Failed test 'Test skipped, swagger-cli missing'
    #   at xt/api.t line 100.
    # Looks like you failed 1 test of 1.
not ok 3 - The spec passes the swagger-cli validation
# Subtest: tags tests

#   Failed test 'The spec passes the swagger-cli validation'
#   at xt/api.t line 102.
    1..1
    ok 1 - No tag errors in the spec
ok 4 - tags tests
# Looks like you failed 1 test of 4.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/4 subtests 

Test Summary Report
-------------------
xt/api.t (Wstat: 256 (exited 1) Tests: 4 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1
Files=1, Tests=4,  4 wallclock secs ( 0.02 usr  0.01 sys +  3.60 cusr  0.20 csys =  3.83 CPU)
Result: FAIL

[3] yarn install output
~~~~~~~~~~~~~~~~~~~~~~~

yarn install
yarn install v1.22.22
[1/4] Resolving packages...
warning swagger-cli > @apidevtools/swagger-cli@4.0.4: This package has been abandoned. Please switch to using the actively maintained @redocly/cli
[2/4] Fetching packages...
[3/4] Linking dependencies...
warning " > bootstrap@4.6.2" has unmet peer dependency "jquery@1.9.1 - 3".
warning " > bootstrap@4.6.2" has unmet peer dependency "popper.js@^1.16.1".
warning " > eslint-config-prettier@9.1.0" has unmet peer dependency "eslint@>=7.0.0".
warning " > eslint-plugin-prettier@5.1.3" has unmet peer dependency "eslint@>=8.0.0".
warning " > eslint-plugin-prettier@5.1.3" has unmet peer dependency "prettier@>=3.0.0".
warning "swagger-cli > @apidevtools/swagger-cli > @apidevtools/swagger-parser@10.1.0" has unmet peer dependency "openapi-types@>=7".
[4/4] Building fresh packages...
success Saved lockfile.
Done in 34.47s.

[4] Tests fail after both patches apply and a reset_all:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

prove xt/api.t -v
xt/api.t .. 
1..4
ok 1 - GET /api/v1/
ok 2
# Subtest: The spec passes the swagger-cli validation
    1..1
    not ok 1 - Validation exit code is 0

    #   Failed test 'Validation exit code is 0'
    #   at xt/api.t line 97.
    #          got: '256'
    #     expected: '0'
    # Validation failed. /paths/libraries/{library_id}/cash_registers/get is missing path parameter(s) for {library_id}
    # Looks like you failed 1 test of 1.
not ok 3 - The spec passes the swagger-cli validation

#   Failed test 'The spec passes the swagger-cli validation'
#   at xt/api.t line 102.
# Subtest: tags tests
    1..1
    ok 1 - No tag errors in the spec
ok 4 - tags tests
# Looks like you failed 1 test of 4.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/4 subtests 

Test Summary Report
-------------------
xt/api.t (Wstat: 256 (exited 1) Tests: 4 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1
Files=1, Tests=4,  5 wallclock secs ( 0.01 usr  0.00 sys +  4.21 cusr  0.30 csys =  4.52 CPU)
Result: FAIL

Yes, the tests are failing, it's "expected" :)

They were skipped on Jenkins and so we missed the failure. With this test back we will be able to catch the problems and fix them on a follow-up bug (which should be pushed along with this one IMO).

Created attachment 168784 [details] [review]
Bug 37302: Set test to failed if swagger-cli missing

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 168785 [details] [review]
Bug 37302: Add swagger-cli to devDependancies

This patch adds swagger-cli 4.0.4+ to the devDependancies section of
package.json. This should ensure it gets installed when appropriate

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 169232 [details] [review]
Bug 37302: Set test to failed if swagger-cli missing

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 169233 [details] [review]
Bug 37302: Add swagger-cli to devDependancies

This patch adds swagger-cli 4.0.4+ to the devDependancies section of
package.json. This should ensure it gets installed when appropriate

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant

Created attachment 169450 [details] [review]
Bug 37302: (follow-up) Update yarn.lock

We were having some issues with package building because we missed the
update to the yarn.lock file here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 169473 [details] [review]
Bug 37302: (follow-up) Update yarn.lock

We were having some issues with package building because we missed the
update to the yarn.lock file here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: mjames <mjames@kohaaloha.com>

(In reply to Mason James from comment #17)
> Created attachment 169473 [details] [review] [review]
> Bug 37302: (follow-up) Update yarn.lock
> 
> We were having some issues with package building because we missed the
> update to the yarn.lock file here.
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> Signed-off-by: mjames <mjames@kohaaloha.com>

tested OK, signing off! many thanks Martin

(In reply to Mason James from comment #17)
> Created attachment 169473 [details] [review] [review]
> Bug 37302: (follow-up) Update yarn.lock
> 
> We were having some issues with package building because we missed the
> update to the yarn.lock file here.
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> Signed-off-by: mjames <mjames@kohaaloha.com>

if you are curious about invoking the problem in KTD...

 cd /kohadevbox/koha
 rm -rf ../node_modules
 ./build-resources.PL

Backported to 24.05.x for upcoming 24.05.04

(In reply to David Nind from comment #9)
> # Subtest: The spec passes the swagger-cli validation
>     1..1
>     not ok 1 - Validation exit code is 0
> 
>     #   Failed test 'Validation exit code is 0'
>     #   at xt/api.t line 97.
>     #          got: '256'
>     #     expected: '0'
>     # Validation failed. /paths/libraries/{library_id}/cash_registers/get is
> missing path parameter(s) for {library_id}
>     # Looks like you failed 1 test of 1.
> not ok 3 - The spec passes the swagger-cli validation
> 
> #   Failed test 'The spec passes the swagger-cli validation'
> #   at xt/api.t line 102.

I don't understand why there is no change to the api specs in these patches.
Initially the tests were failing in ktd (but we didn't notice because Jenkins was skipping them).

Why is Jenkins happy and the tests passing in ktd now whereas we have not adjusted the spec files?

I wait for backport in 23.11.x

(In reply to Jonathan Druart from comment #21)
> (In reply to David Nind from comment #9)
> > # Subtest: The spec passes the swagger-cli validation
> >     1..1
> >     not ok 1 - Validation exit code is 0
> > 
> >     #   Failed test 'Validation exit code is 0'
> >     #   at xt/api.t line 97.
> >     #          got: '256'
> >     #     expected: '0'
> >     # Validation failed. /paths/libraries/{library_id}/cash_registers/get is
> > missing path parameter(s) for {library_id}
> >     # Looks like you failed 1 test of 1.
> > not ok 3 - The spec passes the swagger-cli validation
> > 
> > #   Failed test 'The spec passes the swagger-cli validation'
> > #   at xt/api.t line 102.
> 
> I don't understand why there is no change to the api specs in these patches.
> Initially the tests were failing in ktd (but we didn't notice because
> Jenkins was skipping them).
> 
> Why is Jenkins happy and the tests passing in ktd now whereas we have not
> adjusted the spec files?

Has been actually fixed by bug 36481:
Bug 36481: (follow-up) Add missing library_id parameter