Description
Jonathan Druart
2025-06-18 21:10:48 UTC
Created attachment 183372 [details] [review] Bug 40174: Allow proper E2E testing with Cypress This is the first step toward implementing true end-to-end testing with Cypress. Until now, we have been mocking responses using cy.intercept(), but this approach can lead to confusion. In some cases, we have even had to mock global JavaScript variables, which makes the code unnecessarily complex and unconventional (e.g. win.categories_map). All the bug reports listed under this tree have helped lay the groundwork for this patch. With it, we are able to build mock objects (via plugin mockData) and insert them directly into the database (insertData). Once the tests are complete, we’ll restore the database to its previous state by removing any data generated during the tests. The tests in KohaTable/Holdings_spec.ts have been updated to leverage this new setup. Additionally, a caching mechanism has been added to prevent _id attributes from being generated with the same values (a rare issue we occasionally encountered). While this is still not a perfect solution, it introduces a solid foundation for future improvements. Test plan: All Cypress tests must pass Created attachment 183373 [details] [review] Bug 40174: Introduce Cypress auth plugin So we can easily retrieve the value for the Authorization header. Created attachment 183374 [details] [review] Bug 40174: Use api-client I attempted to test, but one of the dependent bugs doesn't apply: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 40173: Make http-client reusable from Cypress tests Applying: Bug 40173: Return the response early if the caller needs it Applying: Bug 40173: Add test for the api-client Cypress plugin Using index info to reconstruct a base tree... M t/cypress/plugins/index.js Falling back to patching base and 3-way merge... Auto-merging t/cypress/plugins/index.js CONFLICT (content): Merge conflict in t/cypress/plugins/index.js error: Failed to merge in the changes. Patch failed at 0001 Bug 40173: Add test for the api-client Cypress plugin However, it applies fine if I just apply it by itself. Yes the tree is a bit complicated. If you want to have everything applied you can have a look at the branch `bug_40180` on my gitlab repo. Created attachment 183851 [details] Bug 40174 (and related bugs) - Test failues.txt I tested the bug_40180 branch from your repository. This is how I tested things: 1. Added the remote: git remote add joubu https://gitlab.com/joubu/Koha 2. Fetched the remote: git fetch joubu 3. Checked out the branch from the remote: git checkout -b bug_40180 joubu/bug_40180 4. yarn build 5. restart_all (maybe this should be a reset_all?) 6. Enabled the RESTBasicAuth system preference. 7. Ran individual tests for the dependent and related bugs - see [1] for the list 8. Ran all the cypress tests: perl /kohadevbox/misc4dev/run_tests.pl --run-cypress-tests-only - ERM/DataProviders_spec.ts and ERM/UsageReports_spec.ts failed, but I think they've been fixed now - KohaTable/Holdings_spec.ts sometimes failed for me [2] - KohaTable/KohaTable_spec.ts sometimes failed for me [3] 9. The tests related to bugs 40301 and 40320 failed for me. [1] All the interrelated bugs ============================= 40168 - Signed off - yarn cypress run --spec t/cypress/integration/KohaTable/Holdings_spec.ts 40169 - Signed off - yarn cypress run --spec t/cypress/integration/t/mockData.ts 40170 - yarn cypress run --spec t/cypress/integration/Auth/csrf.ts - yarn cypress run --spec t/cypress/integration/Islands/AcquisitionsMenu_spec.ts - yarn cypress run --spec t/cypress/integration/KohaTable/Holdings_spec.ts - yarn cypress run --spec t/cypress/integration/KohaTable/PatronSearch_spec.ts - yarn cypress run --spec t/cypress/integration/t/db.ts 40172 (Signed off) - No additional tests to run 40173 - yarn cypress run --spec t/cypress/integration/t/api-client.ts 40174 - yarn cypress run --spec t/cypress/integration/KohaTable/Holdings_spec.ts (failed??? passed when run before on 40168) - yarn cypress run --spec t/cypress/integration/t/insertData.ts 40175 - OMNIBUS bug (no actual patches) 40176 (Passed QA) - prove xt/api.t 40177 (Passed QA) - prove xt/api.t 40178 (Passed QA) - provec xt/api.t 40179 (Passed QA) - prove xt/api.t 40180 - yarn cypress run --spec t/cypress/integration/KohaTable/Holdings_spec.ts - yarn cypress run --spec t/cypress/integration/KohaTable/PendingHolds_spec.ts - yarn cypress run --spec t/cypress/integration/t/insertData.ts 40181 - Applied on top - yarn cypress run --spec t/cypress/integration/t/insertData.ts 40301 - Applied on top as well as 40281: both tests failed with and without 40281 - yarn cypress run --spec t/cypress/integration/t/insertData.ts - yarn cypress run --spec t/cypress/integration/KohaTable/CirculationHistory_spec.ts 40319 - Applied on top: OK - Follow the test plan in the bug (no tests to run yet) 40320 - Applied on top: tests fail for me [5] - Follow the test plan - Make sure the display of city, state and zipcode is the same as before the patch - yarn cypress run --spec t/cypress/integration/Patron/Address.ts [2], [3], [4], [5] - see attachment to the bug =============================================== (In reply to David Nind from comment #6) > Created attachment 183851 [details] > Bug 40174 (and related bugs) - Test failues.txt > > I tested the bug_40180 branch from your repository. > > This is how I tested things: > 1. Added the remote: git remote add joubu https://gitlab.com/joubu/Koha > 2. Fetched the remote: git fetch joubu > 3. Checked out the branch from the remote: git checkout -b bug_40180 > joubu/bug_40180 > 4. yarn build > 5. restart_all (maybe this should be a reset_all?) Yes, `reset_all` or `yarn api:bundle && restart_all` > 6. Enabled the RESTBasicAuth system preference. > 7. Ran individual tests for the dependent and related bugs - see [1] for the > list > 8. Ran all the cypress tests: perl /kohadevbox/misc4dev/run_tests.pl > --run-cypress-tests-only > - ERM/DataProviders_spec.ts and ERM/UsageReports_spec.ts failed, but I > think they've been fixed now > - KohaTable/Holdings_spec.ts sometimes failed for me [2] > - KohaTable/KohaTable_spec.ts sometimes failed for me [3] I think there are failing because you have not rebuilt the swapper spec (`yarn api:bundle` then restarted plack, or `reset_all`). In theory they could still fail, but with way less reproducibility (without the spec regenerated, branchcodes are generated using only 1 letter, and it clashes if it starts with the first letter of an existing library (eg. C and CPL in your case). With the new changes it will be 10 letters long and we won't have such clashes. > 9. The tests related to bugs 40301 and 40320 failed for me. I would suggest to have the first batch (up to bug 40181) pushed before we focus on the other ones :) Created attachment 183929 [details] [review] Bug 40174: Deal with enum Data too long for column 'checkprevcheckout' Created attachment 183978 [details] [review] Bug 40174: Set minLength to 3 When we generate several objects we often get "Duplicate ID" from the server. This mostly happens when the tests failed previously and the objects have not been removed from the DB. Created attachment 183980 [details] [review] Bug 40174: Set minLength to 3 When we generate several objects we often get "Duplicate ID" from the server. This mostly happens when the tests failed previously and the objects have not been removed from the DB. The second version of the last patch fixes mockData.ts Created attachment 183981 [details] [review] Bug 40174: Require RESTBasicAuth turned ON From MM thread https://chat.koha-community.org/koha-community/pl/aptoodssm7gu8x491onfgxs1xr Cypress tests need RESTBasicAuth - I’m stuck in a chicken-and-egg problem being eaten by its own tail... I need advice. Some Cypress tests need RESTBasicAuth, but it's disabled by default. My plan was to turn it on globally (ie. set before any tests, and reset to its original value after) but we cannot right now: Update in DB could work, but its cached Call the svc script won't work, as we are not logged in yet (and this script require the session cookie) so, I have a lot of solutions, but not sure which one to pick * have a "flush cache" route (smells the rabbit hole) * login to get the cookie, set the syspref via the svc script, logout (ugly, and will slower the cypress tests) * require the pref on for cypress tests * ? I would opt now for the third option, turn it on in Jenkins when we run the cypress tests. And explode for devs when the pref is not set. What do you think? If we push this as it, Jenkins is going to fail. We need to restructure how Jenkins is running the Cypress tests anyway: we need a dedicated job to run them all. We will turn ON the syspref for this job. I'm getting a failure in KohaTable/KohaTable_spec.ts - looks like its to do with the patron category failing to render (line 647 patron-search.inc) (In reply to Matt Blenkinsop from comment #13) > I'm getting a failure in KohaTable/KohaTable_spec.ts - looks like its to do > with the patron category failing to render (line 647 patron-search.inc) Booh, I've the fix on the tree, but not attached here! Bug 40344: Fix KohaTable_spec.ts Moving it here now. (In reply to Jonathan Druart from comment #14) > (In reply to Matt Blenkinsop from comment #13) > > I'm getting a failure in KohaTable/KohaTable_spec.ts - looks like its to do > > with the patron category failing to render (line 647 patron-search.inc) > > Booh, I've the fix on the tree, but not attached here! > Bug 40344: Fix KohaTable_spec.ts > > Moving it here now. Hum it's on its own bug actually, adding the dependency instead then. Created attachment 184103 [details] [review] Bug 40174: Allow proper E2E testing with Cypress This is the first step toward implementing true end-to-end testing with Cypress. Until now, we have been mocking responses using cy.intercept(), but this approach can lead to confusion. In some cases, we have even had to mock global JavaScript variables, which makes the code unnecessarily complex and unconventional (e.g. win.categories_map). All the bug reports listed under this tree have helped lay the groundwork for this patch. With it, we are able to build mock objects (via plugin mockData) and insert them directly into the database (insertData). Once the tests are complete, we’ll restore the database to its previous state by removing any data generated during the tests. The tests in KohaTable/Holdings_spec.ts have been updated to leverage this new setup. Additionally, a caching mechanism has been added to prevent _id attributes from being generated with the same values (a rare issue we occasionally encountered). While this is still not a perfect solution, it introduces a solid foundation for future improvements. Test plan: All Cypress tests must pass Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Created attachment 184104 [details] [review] Bug 40174: Introduce Cypress auth plugin So we can easily retrieve the value for the Authorization header. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Created attachment 184105 [details] [review] Bug 40174: Use api-client Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Created attachment 184106 [details] [review] Bug 40174: Deal with enum Data too long for column 'checkprevcheckout' Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Created attachment 184107 [details] [review] Bug 40174: Set minLength to 3 When we generate several objects we often get "Duplicate ID" from the server. This mostly happens when the tests failed previously and the objects have not been removed from the DB. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Created attachment 184108 [details] [review] Bug 40174: Require RESTBasicAuth turned ON From MM thread https://chat.koha-community.org/koha-community/pl/aptoodssm7gu8x491onfgxs1xr Cypress tests need RESTBasicAuth - I’m stuck in a chicken-and-egg problem being eaten by its own tail... I need advice. Some Cypress tests need RESTBasicAuth, but it's disabled by default. My plan was to turn it on globally (ie. set before any tests, and reset to its original value after) but we cannot right now: Update in DB could work, but its cached Call the svc script won't work, as we are not logged in yet (and this script require the session cookie) so, I have a lot of solutions, but not sure which one to pick * have a "flush cache" route (smells the rabbit hole) * login to get the cookie, set the syspref via the svc script, logout (ugly, and will slower the cypress tests) * require the pref on for cypress tests * ? I would opt now for the third option, turn it on in Jenkins when we run the cypress tests. And explode for devs when the pref is not set. What do you think? If we push this as it, Jenkins is going to fail. We need to restructure how Jenkins is running the Cypress tests anyway: we need a dedicated job to run them all. We will turn ON the syspref for this job. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Created attachment 184195 [details] [review] Bug 40174: Do not generate bigint Prevent "Out of range value for column 'deliverytime'" in a later follow-up bug reports. Created attachment 184327 [details] [review] Bug 40174: Allow proper E2E testing with Cypress This is the first step toward implementing true end-to-end testing with Cypress. Until now, we have been mocking responses using cy.intercept(), but this approach can lead to confusion. In some cases, we have even had to mock global JavaScript variables, which makes the code unnecessarily complex and unconventional (e.g. win.categories_map). All the bug reports listed under this tree have helped lay the groundwork for this patch. With it, we are able to build mock objects (via plugin mockData) and insert them directly into the database (insertData). Once the tests are complete, we’ll restore the database to its previous state by removing any data generated during the tests. The tests in KohaTable/Holdings_spec.ts have been updated to leverage this new setup. Additionally, a caching mechanism has been added to prevent _id attributes from being generated with the same values (a rare issue we occasionally encountered). While this is still not a perfect solution, it introduces a solid foundation for future improvements. Test plan: All Cypress tests must pass Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 184328 [details] [review] Bug 40174: Introduce Cypress auth plugin So we can easily retrieve the value for the Authorization header. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 184329 [details] [review] Bug 40174: Use api-client Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 184330 [details] [review] Bug 40174: Deal with enum Data too long for column 'checkprevcheckout' Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 184331 [details] [review] Bug 40174: Set minLength to 3 When we generate several objects we often get "Duplicate ID" from the server. This mostly happens when the tests failed previously and the objects have not been removed from the DB. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 184332 [details] [review] Bug 40174: Require RESTBasicAuth turned ON From MM thread https://chat.koha-community.org/koha-community/pl/aptoodssm7gu8x491onfgxs1xr Cypress tests need RESTBasicAuth - I’m stuck in a chicken-and-egg problem being eaten by its own tail... I need advice. Some Cypress tests need RESTBasicAuth, but it's disabled by default. My plan was to turn it on globally (ie. set before any tests, and reset to its original value after) but we cannot right now: Update in DB could work, but its cached Call the svc script won't work, as we are not logged in yet (and this script require the session cookie) so, I have a lot of solutions, but not sure which one to pick * have a "flush cache" route (smells the rabbit hole) * login to get the cookie, set the syspref via the svc script, logout (ugly, and will slower the cypress tests) * require the pref on for cypress tests * ? I would opt now for the third option, turn it on in Jenkins when we run the cypress tests. And explode for devs when the pref is not set. What do you think? If we push this as it, Jenkins is going to fail. We need to restructure how Jenkins is running the Cypress tests anyway: we need a dedicated job to run them all. We will turn ON the syspref for this job. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 184333 [details] [review] Bug 40174: Do not generate bigint Prevent "Out of range value for column 'deliverytime'" in a later follow-up bug reports. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> |