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: Needs Signoff --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: dcook
Version: unspecified   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 40172    
Bug Blocks: 40174    
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

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.