Bug 40173

Summary: Reuse http-client from Cypress tests - preparation steps
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: Pushed to main --- QA Contact: Matt Blenkinsop <matt.blenkinsop>
Severity: enhancement    
Priority: P5 - low CC: dcook, martin.renvoize, matt.blenkinsop, pedro.amorim
Version: unspecified   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
25.11.00
Circulation function:
Bug Depends on: 40172    
Bug Blocks: 40174, 40493    
Attachments: Bug 40173: Make http-client reusable from Cypress tests
Bug 40173: Return the response early if the caller needs it
Bug 40173: Add test for the api-client Cypress plugin
Bug 40173: Add test for the api-client Cypress plugin
Bug 40173: Remove Cypress warnings
Bug 40173: Make http-client reusable from Cypress tests
Bug 40173: Return the response early if the caller needs it
Bug 40173: Add test for the api-client Cypress plugin
Bug 40173: Remove Cypress warnings
Bug 40173: Make http-client reusable from Cypress tests
Bug 40173: Return the response early if the caller needs it
Bug 40173: Add test for the api-client Cypress plugin
Bug 40173: Remove Cypress warnings
Bug 40173: Make http-client reusable from Cypress tests
Bug 40173: Return the response early if the caller needs it
Bug 40173: Add test for the api-client Cypress plugin
Bug 40173: Remove Cypress warnings

Description Jonathan Druart 2025-06-18 20:57:22 UTC
We will need to call REST API endpoints from Cypress tests, and more specifically from Cypress's tasks.

It will be useful to reuse what has been done in http-client.js instead of using the low-level fetch JS function.
Comment 1 Jonathan Druart 2025-06-18 21:05:51 UTC
Created attachment 183354 [details] [review]
Bug 40173: Make http-client reusable from Cypress tests

We will need to call REST API endpoints from Cypress tests, and more specifically from Cypress's tasks.

It will be useful to reuse what has been done in http-client.js instead of using the low-level fetch JS function.

1. Add missing get and getAll functions
2. Allow overwrite of return_response and mark_submitting
3. Add a "default" client that will bring flexibility (can call any routes)
4. Add rspack config to generate t/cypress/plugins/dist/api-client.cjs.js that will make it reusable from Cypress's tasks
Comment 2 Jonathan Druart 2025-06-18 21:05:53 UTC
Created attachment 183355 [details] [review]
Bug 40173: Return the response early if the caller needs it

We returned the response too late: if something was wrong we processed
the response already and thrown an error.

This may introduce side-effects but it feels like a correct change.
Comment 3 Jonathan Druart 2025-06-18 21:05:54 UTC
Created attachment 183356 [details] [review]
Bug 40173: Add test for the api-client Cypress plugin
Comment 4 Jonathan Druart 2025-06-18 21:07:26 UTC
No proper test plan here, it affects the low-level http-client so it's hard to make sure it will not introduce bugs.

Same test plan as bug 40172 would already be a good start.
Comment 5 David Cook 2025-06-19 00:04:23 UTC
(In reply to Jonathan Druart from comment #4)
> No proper test plan here, it affects the low-level http-client so it's hard
> to make sure it will not introduce bugs.

I've been thinking about that for a little while now, and I think that we should start making Javascript unit tests. It likes "jest" is already included in "node_modules" for other dependencies. We might need to add "jest-fetch-mock" or "nock" but it would be a good way to test Javascript changes.

I think it would encourage us to write Javascript in more re-usable ways as well. 

Anyway, food for thought.
Comment 6 Jonathan Druart 2025-06-19 06:54:27 UTC
(In reply to David Cook from comment #5)
> (In reply to Jonathan Druart from comment #4)
> > No proper test plan here, it affects the low-level http-client so it's hard
> > to make sure it will not introduce bugs.
> 
> I've been thinking about that for a little while now, and I think that we
> should start making Javascript unit tests. It likes "jest" is already
> included in "node_modules" for other dependencies. We might need to add
> "jest-fetch-mock" or "nock" but it would be a good way to test Javascript
> changes.
> 
> I think it would encourage us to write Javascript in more re-usable ways as
> well. 
> 
> Anyway, food for thought.

I am actually adding test to http-client.js indirectly here, via t/cypress/integration/t/api-client.ts ;)

The more we are going to use it in Cypress tests, the more it is going to be covered by tests.
Comment 7 David Cook 2025-06-19 23:36:45 UTC
(In reply to Jonathan Druart from comment #6)
> I am actually adding test to http-client.js indirectly here, via
> t/cypress/integration/t/api-client.ts ;)

Indirectly being the key word there. 
 
> The more we are going to use it in Cypress tests, the more it is going to be
> covered by tests.

But not really. It's the difference between a unit test and an end to end test. Despite the word "integration" in "t/cypress/integration/t/api-client.ts", it's really an end to end test, and it's just covering the end effects. 

It's like saying we don't need Perl unit tests because we have the Cypress end to end tests.

--

But that's fine. It's the status quo. I think that we could do better than we are currently, but it doesn't need to be right now. It's just an opportunity to try something better.

But the current patches are still a good move forward.
Comment 8 David Cook 2025-06-19 23:39:38 UTC
And the end to end tests are super important. I'm not saying they're not. They definitely are. They're our last line of defence against bugs. 

But then the unit tests should be our first line of defence against bugs. That's all I'm saying.
Comment 9 Jonathan Druart 2025-06-24 09:41:24 UTC
I think we are on the same page, note that I have not used the term "unit tests" ;)
Comment 10 Pedro Amorim 2025-07-09 12:41:23 UTC
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
Comment 11 Jonathan Druart 2025-07-10 07:57:31 UTC
Created attachment 183920 [details] [review]
Bug 40173: Add test for the api-client Cypress plugin
Comment 12 Jonathan Druart 2025-07-10 11:51:42 UTC
Created attachment 183928 [details] [review]
Bug 40173: Remove Cypress warnings

Cypress Warning: Cypress detected that you returned a promise in a test, but also invoked one or more cy commands inside of that promise.
Comment 13 Jonathan Druart 2025-07-11 10:13:50 UTC
Please test and push this along with bug 40174.

Tests from this patchset won't pass without the auth plugin added by bug 40174.
Comment 14 Owen Leonard 2025-07-11 13:05:56 UTC
Created attachment 183994 [details] [review]
Bug 40173: Make http-client reusable from Cypress tests

We will need to call REST API endpoints from Cypress tests, and more
specifically from Cypress's tasks.

It will be useful to reuse what has been done in http-client.js instead
of using the low-level fetch JS function.

1. Add missing get and getAll functions
2. Allow overwrite of return_response and mark_submitting
3. Add a "default" client that will bring flexibility (can call any
   routes)
4. Add rspack config to generate
   t/cypress/plugins/dist/api-client.cjs.js that will make it reusable
   from Cypress's tasks

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 15 Owen Leonard 2025-07-11 13:05:59 UTC
Created attachment 183995 [details] [review]
Bug 40173: Return the response early if the caller needs it

We returned the response too late: if something was wrong we processed
the response already and thrown an error.

This may introduce side-effects but it feels like a correct change.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 16 Owen Leonard 2025-07-11 13:06:01 UTC
Created attachment 183996 [details] [review]
Bug 40173: Add test for the api-client Cypress plugin

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 17 Owen Leonard 2025-07-11 13:06:04 UTC
Created attachment 183997 [details] [review]
Bug 40173: Remove Cypress warnings

Cypress Warning: Cypress detected that you returned a promise in a
test, but also invoked one or more cy commands inside of that promise.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 18 Matt Blenkinsop 2025-07-15 13:38:05 UTC
Created attachment 184098 [details] [review]
Bug 40173: Make http-client reusable from Cypress tests

We will need to call REST API endpoints from Cypress tests, and more
specifically from Cypress's tasks.

It will be useful to reuse what has been done in http-client.js instead
of using the low-level fetch JS function.

1. Add missing get and getAll functions
2. Allow overwrite of return_response and mark_submitting
3. Add a "default" client that will bring flexibility (can call any
   routes)
4. Add rspack config to generate
   t/cypress/plugins/dist/api-client.cjs.js that will make it reusable
   from Cypress's tasks

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Comment 19 Matt Blenkinsop 2025-07-15 13:38:08 UTC
Created attachment 184099 [details] [review]
Bug 40173: Return the response early if the caller needs it

We returned the response too late: if something was wrong we processed
the response already and thrown an error.

This may introduce side-effects but it feels like a correct change.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Comment 20 Matt Blenkinsop 2025-07-15 13:38:10 UTC
Created attachment 184100 [details] [review]
Bug 40173: Add test for the api-client Cypress plugin

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Comment 21 Matt Blenkinsop 2025-07-15 13:38:12 UTC
Created attachment 184101 [details] [review]
Bug 40173: Remove Cypress warnings

Cypress Warning: Cypress detected that you returned a promise in a
test, but also invoked one or more cy commands inside of that promise.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Comment 22 Martin Renvoize (ashimema) 2025-07-18 11:00:23 UTC
Created attachment 184323 [details] [review]
Bug 40173: Make http-client reusable from Cypress tests

We will need to call REST API endpoints from Cypress tests, and more
specifically from Cypress's tasks.

It will be useful to reuse what has been done in http-client.js instead
of using the low-level fetch JS function.

1. Add missing get and getAll functions
2. Allow overwrite of return_response and mark_submitting
3. Add a "default" client that will bring flexibility (can call any
   routes)
4. Add rspack config to generate
   t/cypress/plugins/dist/api-client.cjs.js that will make it reusable
   from Cypress's tasks

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 23 Martin Renvoize (ashimema) 2025-07-18 11:00:26 UTC
Created attachment 184324 [details] [review]
Bug 40173: Return the response early if the caller needs it

We returned the response too late: if something was wrong we processed
the response already and thrown an error.

This may introduce side-effects but it feels like a correct change.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 24 Martin Renvoize (ashimema) 2025-07-18 11:00:28 UTC
Created attachment 184325 [details] [review]
Bug 40173: Add test for the api-client Cypress plugin

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 25 Martin Renvoize (ashimema) 2025-07-18 11:00:31 UTC
Created attachment 184326 [details] [review]
Bug 40173: Remove Cypress warnings

Cypress Warning: Cypress detected that you returned a promise in a
test, but also invoked one or more cy commands inside of that promise.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 26 Martin Renvoize (ashimema) 2025-07-18 11:20:45 UTC
I was testing the whole tree so adding my additional SO lines here too.
Comment 27 Lucas Gass (lukeg) 2025-07-21 20:03:15 UTC
Nice work everyone!

Pushed to main for 25.11