Description
Jonathan Druart
2025-06-18 20:57:22 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 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. Created attachment 183356 [details] [review] Bug 40173: Add test for the api-client Cypress plugin 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. (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. (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. (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. 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. I think we are on the same page, note that I have not used the term "unit tests" ;) 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 Created attachment 183920 [details] [review] Bug 40173: Add test for the api-client Cypress plugin 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. Please test and push this along with bug 40174. Tests from this patchset won't pass without the auth plugin added by bug 40174. 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> 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> Created attachment 183996 [details] [review] Bug 40173: Add test for the api-client Cypress plugin Signed-off-by: Owen Leonard <oleonard@myacpl.org> 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> 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> 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> 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> 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> 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> 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> 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> 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> I was testing the whole tree so adding my additional SO lines here too. Nice work everyone! Pushed to main for 25.11 |