We are not running them (yet) on jenkins and some cypress tests (for erm) are failing.
Created attachment 146343 [details] [review] Bug 32898: Fix Cypress tests
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.
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)
Several tiny but important changes on the second patch. Could I get your opinion on it please?
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)
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)
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.
Created attachment 146643 [details] [review] Bug 32898: Remove unecessary async/await myFetch awaits already
(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!
Created attachment 146723 [details] [review] Bug 32898: Fix Cypress tests Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
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>
Created attachment 146725 [details] [review] Bug 32898: Remove unecessary async/await myFetch awaits already Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Tests were failing, applied patch, tests are now passing.
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.
There is one test failing, GET 500 /api/v1/erm/eholdings/local/titles?_page=1&_per_page=1 no alias 5 get main div[class='dialog alert'] 0 AssertionError Timed out retrying after 4000ms: Expected to find element: main div[class='dialog alert'], but never found it. The error is not raised to the user when we list titles. It's because fetchLocalTitleCount does not call checkError. We are dealing with that correctly in follow-up bugs, I am tempted to comment the test for now and add it back later. What do you think Pedro?
Created attachment 146957 [details] [review] Bug 32898: Avoid hardcoding user/pass in tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
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>
(In reply to Jonathan Druart from comment #15) > There is one test failing, > > GET 500 /api/v1/erm/eholdings/local/titles?_page=1&_per_page=1 > no alias > 5 > get > main div[class='dialog alert'] > 0 > AssertionError > Timed out retrying after 4000ms: Expected to find element: main > div[class='dialog alert'], but never found it. > > > The error is not raised to the user when we list titles. It's because > fetchLocalTitleCount does not call checkError. > We are dealing with that correctly in follow-up bugs, I am tempted to > comment the test for now and add it back later. What do you think Pedro? Forget that, I was using an old version of dist file.
Ready to go, thanks for the patch, Tomas!
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work, thanks everyone! Pushed to 22.11.x for the next release.
Not needed in 22.05.x