Bug 37302 - xt/api.t should fail if swagger-cli is missing
Summary: xt/api.t should fail if swagger-cli is missing
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P1 - high normal (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-07-10 10:19 UTC by Martin Renvoize
Modified: 2024-07-11 10:04 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 37302: Set test to failed if swagger-cli missing (974 bytes, patch)
2024-07-10 10:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37302: Add swagger-cli to devDependancies (805 bytes, patch)
2024-07-10 10:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37302: Set test to failed if swagger-cli missing (1020 bytes, patch)
2024-07-11 10:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 37302: Add swagger-cli to devDependancies (852 bytes, patch)
2024-07-11 10:04 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2024-07-10 10:19:48 UTC
Currently, we skip if the swagger-cli utility is missing.  This was hiding genuine issues in Jenkins, we should make it as a failure with a suitable message to the end user.
Comment 1 Martin Renvoize 2024-07-10 10:20:27 UTC
Created attachment 168708 [details] [review]
Bug 37302: Set test to failed if swagger-cli missing
Comment 2 Martin Renvoize 2024-07-10 10:22:13 UTC
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.
Comment 3 Martin Renvoize 2024-07-10 10:27:31 UTC
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
Comment 4 Jonathan Druart 2024-07-10 11:37:32 UTC
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
Comment 5 Tomás Cohen Arazi 2024-07-10 13:38:26 UTC
(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?
Comment 6 Jonathan Druart 2024-07-10 13:41:48 UTC
(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).
Comment 7 Tomás Cohen Arazi 2024-07-10 13:54:41 UTC
(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.
Comment 8 Jonathan Druart 2024-07-10 14:01:44 UTC
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.
Comment 9 David Nind 2024-07-10 22:46:38 UTC
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
Comment 10 Jonathan Druart 2024-07-11 08:58:39 UTC
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).
Comment 11 David Nind 2024-07-11 10:04:09 UTC
Created attachment 168784 [details] [review]
Bug 37302: Set test to failed if swagger-cli missing

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2024-07-11 10:04:12 UTC
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>