We need UI tests for the table features. We have different kinds of table and a lot of features that must be covered by tests. 3 types of table: * Tables using the REST API (kohaTable) * Tables not using the REST API (KohaTable) * Tables using dataTables constructor directly (should be removed by bug 38255) Features: * Save state * Copy shareable link * Save configuration state on page change * Save search state on page change: * Export * Search (and the 'Clear filters' button) * Columns filters * External filters * etc. Specific tables: * Patrons search * Items list (catalogue/detail.pl) * Items search * Pending requests (should not be different but it is) * Libraries ("classic" using kohaTable)
Created attachment 174877 [details] [review] Bug 38461: Add tests for 'Clear filter'
Created attachment 174878 [details] [review] Bug 38461: Use buildSampleObjects
Created attachment 175004 [details] [review] Bug 38461: Add tests for 'Clear filter'
Created attachment 175005 [details] [review] Bug 38461: Use buildSampleObjects
Created attachment 175006 [details] [review] Bug 38461: Columns - All, is_hidden by default, then shown by user
Created attachment 175007 [details] [review] Bug 38461: Save state ON/OFF
Created attachment 175008 [details] [review] Bug 38461: Refactoring - build_libraries
Created attachment 175009 [details] [review] Bug 38461: Refactoring - mock_table_settings
Created attachment 175010 [details] [review] Bug 38461: Shareable link
Created attachment 175011 [details] [review] Bug 38461: Jump to the configuration page
I am getting random failures, Cypress bugs? Test "Shareable link": * "Write permission denied" (not allowed to write to navigator.clipboard) * "Document is not focused" when trying to click on the "Copy shareable link" button.
Created attachment 175015 [details] [review] Bug 38461: Fix ramdom failures All this is weird. I found this an implemented it: https://stackoverflow.com/questions/69425289/javascript-prompt-cause-document-is-not-focused It was still not enough for reduced the failures. The wait and adding the focus to the window seems ok. If I remove one of those 3 hacks, I still get failures...
(In reply to Jonathan Druart from comment #12) > Created attachment 175015 [details] [review] [review] > Bug 38461: Fix ramdom failures > > All this is weird. > I found this an implemented it: > https://stackoverflow.com/questions/69425289/javascript-prompt-cause- > document-is-not-focused > It was still not enough for reduced the failures. > The wait and adding the focus to the window seems ok. > If I remove one of those 3 hacks, I still get failures... Ho well, I just got the error now.. " > Document is not focused."
Created attachment 175016 [details] [review] Bug 38461: Bypass the clipboard for tests There are known bugs with Cypress trying to play with the clipboard, and I don't manage to make the tests pass consistently (also tried realClick from cypress-real-events) Here I am suggesting to not click on the button, but simply retrieve the url (which needs a bit of work in the existing code). We don't test the "Copied!" tooltip and the behaviour of the button, but at least the tests pass!
Created attachment 175179 [details] [review] Bug 38461: Add tests for catalogue/detail/holdings_table
Obviously more tests are always needed, but this can go as it for now.
I'm not sure if I tested this correctly, but the tests no longer pass for me: 1. Before the patch, the current tests pass: yarn cypress run --spec t/cypress/integration/KohaTable_spec.ts 2. Apply the patch and restart everything. 3. Run: yarn install 4. Rerun the tests - 1 test is failing: 1) Hit all tables catalogue/detail/holdings_table correctly init: ReferenceError: restdefaultpagesize is not defined at Context.eval (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:487:23)
Created attachment 175223 [details] [review] Bug 38461: Add tests for 'Clear filter' Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175224 [details] [review] Bug 38461: Use buildSampleObjects Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175225 [details] [review] Bug 38461: Columns - All, is_hidden by default, then shown by user Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175226 [details] [review] Bug 38461: Save state ON/OFF Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175227 [details] [review] Bug 38461: Refactoring - build_libraries Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175228 [details] [review] Bug 38461: Refactoring - mock_table_settings Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175229 [details] [review] Bug 38461: Shareable link Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175230 [details] [review] Bug 38461: Jump to the configuration page Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175231 [details] [review] Bug 38461: Fix ramdom failures All this is weird. I found this an implemented it: https://stackoverflow.com/questions/69425289/javascript-prompt-cause-document-is-not-focused It was still not enough for reduced the failures. The wait and adding the focus to the window seems ok. If I remove one of those 3 hacks, I still get failures... Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175232 [details] [review] Bug 38461: Bypass the clipboard for tests There are known bugs with Cypress trying to play with the clipboard, and I don't manage to make the tests pass consistently (also tried realClick from cypress-real-events) Here I am suggesting to not click on the button, but simply retrieve the url (which needs a bit of work in the existing code). We don't test the "Copied!" tooltip and the behaviour of the button, but at least the tests pass! Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175233 [details] [review] Bug 38461: Add tests for catalogue/detail/holdings_table Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175234 [details] [review] Bug 38461: Fix multiple case changes that broke variables
It works! :) 25 runs without issue with the default electron browser. And with Bug 38503 with the patches that remove @vue/cli-service and babel. Further reinforcing the idea that there is no dependency on them. It also worked with @vue/cli-service and babel. ------- There were very weird issues with variables being lowercased and thus not matching there declaration. That's the commit I added. Are the patches here the latest version you have locally Jonathan? (in case there is commit mixup) @David: that's the issue you encountered.
(In reply to David Nind from comment #17) > 1) Hit all tables > catalogue/detail/holdings_table > correctly init: > ReferenceError: restdefaultpagesize is not defined > at Context.eval > (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:487:23) Victor fixed it in the last patch. Not sure what happened here. My editor did something wrong when saving the file it seems.
Created attachment 175235 [details] [review] Bug 38461: Fix multiple case changes that broke variables Amended-by: Jonathan Druart - "x-base-total-count": baseTotalCount, - "x-total-count": baseTotalCount, + "X-Base-Total-Count": baseTotalCount, + "X-Total-Count": baseTotalCount,
About comment 11 and it's consequences, I did some experiments: (to hopefully not end up with the same kind of struggles as with Selenium) I tried to change the browser: With Firefox 128 with the fixes for random failures still applied Run 1: One column hidden by default: TypeError: win is undefined at ./t/cypress/integration/KohaTable_spec.ts/</ (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:58:12) Input search bar and clear filter : AssertionError: Timed out retrying after 10000ms: Expected to find content: 'Showing 1 to 20 of 42 entries' within the element: <div.dt-info> but never did. at ./t/cypress/integration/KohaTable_spec.ts/</</ (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:370:56) Run 2: AssertionError: Timed out retrying after 10000ms: Expected to find content: 'Showing 1 to 20 of 42 entries' within the element: <div.dt-info> but never did. at ./t/cypress/integration/KohaTable_spec.ts/</</ (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:370:56) Looks like it consistently not pass and has some random failure of it's own. ----- With Chromium 120 with the fixes for random failures still applied: 2 passes at least. With Chromium 120 with the 2 patches reverted: In 3 runs, that same error: Shareable link: AssertionError: Timed out retrying after 10000ms: Expected to find element: `.tooltip`, but never found it. at Context.eval (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:278:23) So it doesn't pass consistently either. Next lead: update Cypress, it's a bit old. And so is Electron. But before that, I still can't reproduce the comment 11 issues. I'm at 105 runs. So far I got these unrelated issues: -------------------------------------------------------------------- 1) kohaTable (using REST API) Simple tables "before each" hook for "Jump to the configuration page": CypressError: Cypress detected that you returned a promise from a command while also invoking one or more cy commands in that promise. The command that returned the promise was: > `cy.visit()` The cy command you invoked inside the promise was: > `cy.login()` Because Cypress commands are already promise-like, you don't need to wrap them or return your own promise. Cypress will resolve your command with whatever the final Cypress command yields. The reason this is an error instead of a warning is because Cypress internally queues commands serially whereas Promises execute as soon as they are invoked. Attempting to reconcile this would prevent Cypress from ever resolving. https://on.cypress.io/returning-promise-and-commands-in-another-command Because this error occurred during a `before each` hook we are skipping the remaining tests in the current suite: `kohaTable (using REST API)` at cy.<computed> [as login] (http://localhost:8081/__cypress/runner/cypress_runner.js:137643:64) From Your Spec Code: at eval (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:282:24) ---------------------------------------------------- 1) Hit all tables catalogue/detail/holdings_table correctly init: CypressError: The following error originated from your test code, not from Cypress. It was caused by an unhandled promise rejection. > Timed out after waiting `60000ms` for your remote page to load. Your page did not fire its `load` event within `60000ms`. You can try increasing the `pageLoadTimeout` value in `kohadevbox/koha/cypress.config.ts` to wait longer. Browsers will not fire the `load` event until all stylesheets and scripts are done downloading. When this `load` event occurs, Cypress will continue running commands. When Cypress detects uncaught errors originating from your test code it will automatically fail the current test. at timedOutWaitingForPageLoad (http://localhost:8081/__cypress/runner/cypress_runner.js:129355:64) at <unknown> (http://localhost:8081/__cypress/runner/cypress_runner.js:130329:16) at tryCatcher (http://localhost:8081/__cypress/runner/cypress_runner.js:1807:23) at <unknown> (http://localhost:8081/__cypress/runner/cypress_runner.js:4186:41) at tryCatcher (http://localhost:8081/__cypress/runner/cypress_runner.js:1807:23) at Promise._settlePromiseFromHandler (http://localhost:8081/__cypress/runner/cypress_runner.js:1519:31) at Promise._settlePromise (http://localhost:8081/__cypress/runner/cypress_runner.js:1576:18) at Promise._settlePromise0 (http://localhost:8081/__cypress/runner/cypress_runner.js:1621:10) at Promise._settlePromises (http://localhost:8081/__cypress/runner/cypress_runner.js:1697:18) at _drainQueueStep (http://localhost:8081/__cypress/runner/cypress_runner.js:2407:12) at _drainQueue (http://localhost:8081/__cypress/runner/cypress_runner.js:2400:9) at Async._drainQueues (http://localhost:8081/__cypress/runner/cypress_runner.js:2416:5) at Async.drainQueues (http://localhost:8081/__cypress/runner/cypress_runner.js:2286:14) From Your Spec Code: at Context.eval (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:518:19) From Your Spec Code: at Context.eval (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:518:19) ---------------------------------------------------- "before each" hook for "Jump to the configuration page": Timed out retrying after 10000ms + expected - actual -'Session timed out › Log in to Koha › Koha' +'Koha staff interface' at Context.eval (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:56:19) ---------------------------------------------------- Is there hope that none of our different issues happen in CI? So we could try our chances without the commits - Fix ramdom failures (2.89 KB, patch) - Bypass the clipboard for tests to keep the test code the simplest. And add them if the CI shows the same errors. -------------------- I almost forgot: you could try updating cypress&electron and it might help to get away with the simplest version of these patches. And it needs updates anyway because Electron 106 would be Chromium from September 27, 2022 -------------------------- Hopefully there are some things helpful in all this.
(In reply to Jonathan Druart from comment #31) > (In reply to David Nind from comment #17) > > 1) Hit all tables > > catalogue/detail/holdings_table > > correctly init: > > ReferenceError: restdefaultpagesize is not defined > > at Context.eval > > (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:487:23) > > Victor fixed it in the last patch. Not sure what happened here. My editor > did something wrong when saving the file it seems. Thanks Jonathan, and Victor!
535 runs in total without any relevant error it seems >_< (without "Fix ramdom failures" and "Bypass the clipboard for tests") Turns out a slow PC isn't enough to reproduce all the random fails :( One more new wack error: Timeout in localhost?! --------------------------- Shareable link: CypressError: `cy.visit()` failed trying to load: http://localhost:8081/cgi-bin/koha/admin/branches.pl We attempted to make an http request to this URL but the request failed without a response. We received this error at the network level: > Error: ESOCKETTIMEDOUT Common situations why this would fail: - you don't have internet access - you forgot to run / boot your web server - your web server isn't accessible - you have weird network configuration settings on your computer at <unknown> (http://localhost:8081/__cypress/runner/cypress_runner.js:130312:74) at visitFailedByErr (http://localhost:8081/__cypress/runner/cypress_runner.js:129704:12) at <unknown> (http://localhost:8081/__cypress/runner/cypress_runner.js:130311:11) at tryCatcher (http://localhost:8081/__cypress/runner/cypress_runner.js:1807:23) at Promise._settlePromiseFromHandler (http://localhost:8081/__cypress/runner/cypress_runner.js:1519:31) at Promise._settlePromise (http://localhost:8081/__cypress/runner/cypress_runner.js:1576:18) at Promise._settlePromise0 (http://localhost:8081/__cypress/runner/cypress_runner.js:1621:10) at Promise._settlePromises (http://localhost:8081/__cypress/runner/cypress_runner.js:1697:18) at _drainQueueStep (http://localhost:8081/__cypress/runner/cypress_runner.js:2407:12) at _drainQueue (http://localhost:8081/__cypress/runner/cypress_runner.js:2400:9) at Async._drainQueues (http://localhost:8081/__cypress/runner/cypress_runner.js:2416:5) at Async.drainQueues (http://localhost:8081/__cypress/runner/cypress_runner.js:2286:14) From Your Spec Code: at Context.eval (webpack://koha/./t/cypress/integration/KohaTable_spec.ts:239:19) --------------------------- And I got again the "Cypress detected that you returned a promise from a command while also invoking one or more cy commands in that promise. " ------
Anyway, tldr it works and can be QAed. And there is hope to try updating cypress&electron (old from September 2022) to help to get away with the simplest version of these patches. But I can't try that so only can Jonathan or someone else getting also the issues from comment 11 (without the 2 relevant patches) --- Now that I think of it, my failed attempts with firefox and chromium are invalid because I kept a cypress older than the browsers... ---- minor errata: In a previous comment I copy pasted "Fix ramdom failures (2.89 KB, patch)" the last part is a mistake and not an emphasis the size of the patch. It's not relevant.
additional info: still works if forgetting to upgrade node from 18 to 22. That's still a good opportunity to upgrade though. (to test, edit version in /etc/apt/sources.list.d/nodesource.list and then `sudo apt update;sudo install nodejs`)
(In reply to Victor Grousset/tuxayo from comment #33) > Is there hope that none of our different issues happen in CI? So we could > try our chances without the commits > - Fix ramdom failures (2.89 KB, patch) > - Bypass the clipboard for tests > to keep the test code the simplest. > And add them if the CI shows the same errors. No, I don't want random failures. I need those tests to provide more DT enhancements, I don't want to fight random failures. I will deal with them later.
Created attachment 175289 [details] [review] Bug 38461: Fix random failures All this is weird. I found this and implemented it: https://stackoverflow.com/questions/69425289/javascript-prompt-cause-document-is-not-focused It was still not enough to reduce the failures. The wait and adding the focus to the window seems ok. If I remove one of those 3 hacks, I still get failures... Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175290 [details] [review] Bug 38461: Bypass the clipboard for tests There are known bugs with Cypress trying to play with the clipboard, and I don't manage to make the tests pass consistently (also tried realClick from cypress-real-events) Here I am suggesting to not click on the button, but simply retrieve the url (which needs a bit of work in the existing code). We don't test the "Copied!" tooltip and the behaviour of the button, but at least the tests pass! Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175291 [details] [review] Bug 38461: Add tests for catalogue/detail/holdings_table Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 175292 [details] [review] Bug 38461: Fix multiple case changes that broke variables Amended-by: Jonathan Druart - "x-base-total-count": baseTotalCount, - "x-total-count": baseTotalCount, + "X-Base-Total-Count": baseTotalCount, + "X-Total-Count": baseTotalCount,
(fixed typos)
Test plan: 1. Apply patches and dependencies 2. Install node dependencies that come from bug 38503 sudo yarn install --modules-folder /kohadevbox/node_modules 3. cypress run --spec t/cypress/integration/KohaTable_spec.ts
Created attachment 176082 [details] [review] Bug 38461: Add tests for 'Clear filter' Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176083 [details] [review] Bug 38461: Use buildSampleObjects Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176084 [details] [review] Bug 38461: Columns - All, is_hidden by default, then shown by user Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176085 [details] [review] Bug 38461: Save state ON/OFF Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176086 [details] [review] Bug 38461: Refactoring - build_libraries Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176087 [details] [review] Bug 38461: Refactoring - mock_table_settings Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176088 [details] [review] Bug 38461: Shareable link Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176089 [details] [review] Bug 38461: Jump to the configuration page Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176090 [details] [review] Bug 38461: Fix random failures All this is weird. I found this and implemented it: https://stackoverflow.com/questions/69425289/javascript-prompt-cause-document-is-not-focused It was still not enough to reduce the failures. The wait and adding the focus to the window seems ok. If I remove one of those 3 hacks, I still get failures... Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176091 [details] [review] Bug 38461: Bypass the clipboard for tests There are known bugs with Cypress trying to play with the clipboard, and I don't manage to make the tests pass consistently (also tried realClick from cypress-real-events) Here I am suggesting to not click on the button, but simply retrieve the url (which needs a bit of work in the existing code). We don't test the "Copied!" tooltip and the behaviour of the button, but at least the tests pass! Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176092 [details] [review] Bug 38461: Add tests for catalogue/detail/holdings_table Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176093 [details] [review] Bug 38461: Fix multiple case changes that broke variables Amended-by: Jonathan Druart - "x-base-total-count": baseTotalCount, - "x-total-count": baseTotalCount, + "X-Base-Total-Count": baseTotalCount, + "X-Total-Count": baseTotalCount, Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Code flows nicely and it's great to see the tests being added.. minor refactoring inline makes reasonable sense. Passing QA
Pushed for 25.05! Well done everyone, thank you!
Created attachment 176793 [details] [review] Bug 38461: Do not rely on biblio_id=1 It might not exist! This code is not trivial and should ideally be moved to somewhere it could be reused easily. But good enough for now to fix the failing tests.
Follow-up patch "Do not rely on biblio_id=1" pushed to main.
Created attachment 176846 [details] [review] Bug 38461: (follow-up) Do not rely on biblio_id=1 19:05:50 koha_1 | "error": "Basic authentication disabled" We don't need to pass the auth in this request, we are authenticated already.
Bug 38461: (follow-up) Do not rely on biblio_id=1 pushed to main.