Bug 32898 - Cypress tests are failing
Summary: Cypress tests are failing
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 32923
  Show dependency treegraph
 
Reported: 2023-02-07 16:28 UTC by Jonathan Druart
Modified: 2024-02-14 13:21 UTC (History)
7 users (show)

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


Attachments
Bug 32898: Fix Cypress tests (8.44 KB, patch)
2023-02-07 16:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32898: ERM - Improve fetch (33.38 KB, patch)
2023-02-08 11:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32898: ERM - Improve fetch (32.98 KB, patch)
2023-02-08 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32898: ERM - Improve fetch (32.88 KB, patch)
2023-02-08 16:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32898: Remove unecessary async/await (5.96 KB, patch)
2023-02-14 16:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32898: Fix Cypress tests (8.49 KB, patch)
2023-02-16 11:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32898: ERM - Improve fetch (32.91 KB, patch)
2023-02-16 11:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32898: Remove unecessary async/await (6.02 KB, patch)
2023-02-16 11:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32898: Avoid hardcoding user/pass in tests (3.78 KB, patch)
2023-02-20 14:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32898: Avoid hardcoding user/pass in tests (4.38 KB, patch)
2023-02-20 14:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32898: Fix Cypress tests (8.56 KB, patch)
2023-02-20 14:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32898: ERM - Improve fetch (32.99 KB, patch)
2023-02-20 14:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32898: Remove unecessary async/await (6.08 KB, patch)
2023-02-20 14:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32898: Avoid hardcoding user/pass in tests (4.38 KB, patch)
2023-02-20 14:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-02-07 16:28:48 UTC
We are not running them (yet) on jenkins and some cypress tests (for erm) are failing.
Comment 1 Jonathan Druart 2023-02-07 16:54:36 UTC
Created attachment 146343 [details] [review]
Bug 32898: Fix Cypress tests
Comment 2 Jonathan Druart 2023-02-07 16:55:45 UTC
There is still Titles_spec.ts that is failing. It's caused by:

    await fetch(apiUrl + "?" + new URLSearchParams(params))
        //.then(checkError)
        .then(
            (response) =>
                (count_local_titles = response.headers.get("X-Total-Count"))
        );

The error is not displayed if a 500 is encountered. However I didn't manage to fix it correctly.
Comment 3 Jonathan Druart 2023-02-08 11:08:16 UTC
Created attachment 146384 [details] [review]
Bug 32898: ERM - Improve fetch

... and fix cypress tests

The main problem was that there were no more checkError call for fetchLocalTitleCount
and the error was not displayed on the UI.

This commit is doing several things:
- Refactor fetch.js to call the same fetch method (myFetch) in order to
deal with the error in a single place
- Have a new myFetchTotal JS function to get the X-Total-Count header
from the response and get the benefit of the previous refactoring
- Rename fetchCountLocalPackages with fetchLocalPackagesCount to match fetchLocalTitleCount
- Use fetchLocalPackagesCount on the Package list view
- Use checkError from components using fetch directly. Ideally the could
should be moved to fetch.js and myFetch should be used.
- checkError get a new flag in parameter to ask for the response instead
of the result (ie. the JSON of the response). That's useful when we need
the response status or the headers
- Make checkError throw a proper exception to stop propation. This
modify the error we display on the UI, it's why we have the following
changes in cypress tests:
-            "Something went wrong: Internal Server Error"
+            "Something went wrong: Error: Internal Server Error"
That's not ideal, we should have our own JS exception to avoid that
"Error" (coming from the toString call on the error)
Comment 4 Jonathan Druart 2023-02-08 11:10:44 UTC
Several tiny but important changes on the second patch. Could I get your opinion on it please?
Comment 5 Jonathan Druart 2023-02-08 11:15:15 UTC
Created attachment 146385 [details] [review]
Bug 32898: ERM - Improve fetch

... and fix cypress tests

The main problem was that there were no more checkError call for fetchLocalTitleCount
and the error was not displayed on the UI.

This commit is doing several things:
- Refactor fetch.js to call the same fetch method (myFetch) in order to
deal with the error in a single place
- Have a new myFetchTotal JS function to get the X-Total-Count header
from the response and get the benefit of the previous refactoring
- Rename fetchCountLocalPackages with fetchLocalPackagesCount to match fetchLocalTitleCount
- Use fetchLocalPackagesCount on the Package list view
- Use checkError from components using fetch directly. Ideally the could
should be moved to fetch.js and myFetch should be used.
- checkError get a new flag in parameter to ask for the response instead
of the result (ie. the JSON of the response). That's useful when we need
the response status or the headers
- Make checkError throw a proper exception to stop propation. This
modify the error we display on the UI, it's why we have the following
changes in cypress tests:
-            "Something went wrong: Internal Server Error"
+            "Something went wrong: Error: Internal Server Error"
That's not ideal, we should have our own JS exception to avoid that
"Error" (coming from the toString call on the error)
Comment 6 Jonathan Druart 2023-02-08 16:24:01 UTC
Created attachment 146399 [details] [review]
Bug 32898: ERM - Improve fetch

... and fix cypress tests

The main problem was that there were no more checkError call for fetchLocalTitleCount
and the error was not displayed on the UI.

This commit is doing several things:
- Refactor fetch.js to call the same fetch method (myFetch) in order to
deal with the error in a single place
- Have a new myFetchTotal JS function to get the X-Total-Count header
from the response and get the benefit of the previous refactoring
- Rename fetchCountLocalPackages with fetchLocalPackagesCount to match fetchLocalTitleCount
- Use fetchLocalPackagesCount on the Package list view
- Use checkError from components using fetch directly. Ideally the could
should be moved to fetch.js and myFetch should be used.
- checkError get a new flag in parameter to ask for the response instead
of the result (ie. the JSON of the response). That's useful when we need
the response status or the headers
- Make checkError throw a proper exception to stop propation. This
modify the error we display on the UI, it's why we have the following
changes in cypress tests:
-            "Something went wrong: Internal Server Error"
+            "Something went wrong: Error: Internal Server Error"
That's not ideal, we should have our own JS exception to avoid that
"Error" (coming from the toString call on the error)
Comment 7 Paul Derscheid 2023-02-14 15:09:09 UTC
A more idiomatic way to write

    response && response.status === 2XX

would be

    response?.status === 2XX ?? false

I think this is easier to read when scanning through the code. As this is a matter of taste, I'll let you be the judge.

Additionally I don't think you need the return await. You await already in the myFetch function so you can just return.
Comment 8 Jonathan Druart 2023-02-14 16:17:24 UTC
Created attachment 146643 [details] [review]
Bug 32898: Remove unecessary async/await

myFetch awaits already
Comment 9 Jonathan Druart 2023-02-14 16:18:14 UTC
(In reply to Paul Derscheid from comment #7)
> A more idiomatic way to write
> 
>     response && response.status === 2XX
> 
> would be
> 
>     response?.status === 2XX ?? false
> 
> I think this is easier to read when scanning through the code. As this is a
> matter of taste, I'll let you be the judge.

I didn't know about that syntax. I am not sure I am ready yet :D

> Additionally I don't think you need the return await. You await already in
> the myFetch function so you can just return.

I noticed that but was lazy to try if removing them was ok. Thanks for confirming!
Comment 10 Pedro Amorim 2023-02-16 11:51:08 UTC
Created attachment 146723 [details] [review]
Bug 32898: Fix Cypress tests

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 11 Pedro Amorim 2023-02-16 11:51:11 UTC
Created attachment 146724 [details] [review]
Bug 32898: ERM - Improve fetch

... and fix cypress tests

The main problem was that there were no more checkError call for fetchLocalTitleCount
and the error was not displayed on the UI.

This commit is doing several things:
- Refactor fetch.js to call the same fetch method (myFetch) in order to
deal with the error in a single place
- Have a new myFetchTotal JS function to get the X-Total-Count header
from the response and get the benefit of the previous refactoring
- Rename fetchCountLocalPackages with fetchLocalPackagesCount to match fetchLocalTitleCount
- Use fetchLocalPackagesCount on the Package list view
- Use checkError from components using fetch directly. Ideally the could
should be moved to fetch.js and myFetch should be used.
- checkError get a new flag in parameter to ask for the response instead
of the result (ie. the JSON of the response). That's useful when we need
the response status or the headers
- Make checkError throw a proper exception to stop propation. This
modify the error we display on the UI, it's why we have the following
changes in cypress tests:
-            "Something went wrong: Internal Server Error"
+            "Something went wrong: Error: Internal Server Error"
That's not ideal, we should have our own JS exception to avoid that
"Error" (coming from the toString call on the error)

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 12 Pedro Amorim 2023-02-16 11:51:14 UTC
Created attachment 146725 [details] [review]
Bug 32898: Remove unecessary async/await

myFetch awaits already

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 13 Pedro Amorim 2023-02-16 11:55:46 UTC
Tests were failing, applied patch, tests are now passing.
Comment 14 Pedro Amorim 2023-02-16 11:59:17 UTC
Cypress testing notes:
- update baseUrl setting in cypress.jon
- outside k-t-d, on koha source:
-- yarn install
-- yarn run cypress open

Good changes on fetch that ultimately initiated the thought process for some of the other bugs in the dependency tree.
Comment 15 Jonathan Druart 2023-02-20 08:14:20 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2023-02-20 14:08:01 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2023-02-20 14:39:23 UTC
Created attachment 146960 [details] [review]
Bug 32898: Avoid hardcoding user/pass in tests

This patch makes the authentication parameters be configurable through
env. Before this, it was hardcoded to koha/koha, which only works if the
ktd defaults haven't been tweaked.

If no env is defined, it fallsback to koha/koha.

In order to pass the ktd configured params, the tests need to be run
like:

$ yarn cypress run --env KOHA_USER=$KOHA_USER,KOHA_PASS=$KOHA_PASS

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi 2023-02-20 14:42:57 UTC
Created attachment 146961 [details] [review]
Bug 32898: Fix Cypress tests

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2023-02-20 14:43:01 UTC
Created attachment 146962 [details] [review]
Bug 32898: ERM - Improve fetch

... and fix cypress tests

The main problem was that there were no more checkError call for fetchLocalTitleCount
and the error was not displayed on the UI.

This commit is doing several things:
- Refactor fetch.js to call the same fetch method (myFetch) in order to
deal with the error in a single place
- Have a new myFetchTotal JS function to get the X-Total-Count header
from the response and get the benefit of the previous refactoring
- Rename fetchCountLocalPackages with fetchLocalPackagesCount to match fetchLocalTitleCount
- Use fetchLocalPackagesCount on the Package list view
- Use checkError from components using fetch directly. Ideally the could
should be moved to fetch.js and myFetch should be used.
- checkError get a new flag in parameter to ask for the response instead
of the result (ie. the JSON of the response). That's useful when we need
the response status or the headers
- Make checkError throw a proper exception to stop propation. This
modify the error we display on the UI, it's why we have the following
changes in cypress tests:
-            "Something went wrong: Internal Server Error"
+            "Something went wrong: Error: Internal Server Error"
That's not ideal, we should have our own JS exception to avoid that
"Error" (coming from the toString call on the error)

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2023-02-20 14:43:04 UTC
Created attachment 146963 [details] [review]
Bug 32898: Remove unecessary async/await

myFetch awaits already

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2023-02-20 14:43:07 UTC
Created attachment 146964 [details] [review]
Bug 32898: Avoid hardcoding user/pass in tests

This patch makes the authentication parameters be configurable through
env. Before this, it was hardcoded to koha/koha, which only works if the
ktd defaults haven't been tweaked.

If no env is defined, it fallsback to koha/koha.

In order to pass the ktd configured params, the tests need to be run
like:

$ yarn cypress run --env KOHA_USER=$KOHA_USER,KOHA_PASS=$KOHA_PASS

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Jonathan Druart 2023-02-21 07:58:45 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2023-02-21 07:59:20 UTC
Ready to go, thanks for the patch, Tomas!
Comment 24 Tomás Cohen Arazi 2023-02-21 12:18:33 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 25 Jacob O'Mara 2023-02-24 11:33:12 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 26 Jacob O'Mara 2023-02-24 11:36:23 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 27 Lucas Gass 2023-03-01 14:53:49 UTC
Not needed in 22.05.x