Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Maintenance] improve testing utils and add delete-user-file test #421

Merged
merged 5 commits into from
Aug 15, 2024

Conversation

tcrasset
Copy link
Contributor

@tcrasset tcrasset commented Aug 15, 2024

Adding integration tests is a pre-requisite to #420, as refactoring might break some stuff without us knowing, and manually doing the actions on the front-end is a pain.

I wanted to create a small PR that shows the value of adding integration tests.
I kept it small on purpose so that I wouldn't have done this for nothing if it gets rejected.

My plan is that this gets accepted, and I/we start adding integration test on all/major endpoints, so that we can more readily refactor the backend to be inline with #420.

@actual-github-bot actual-github-bot bot changed the title improve testing utils and add delete-user-file test [WIP] improve testing utils and add delete-user-file test Aug 15, 2024
@tcrasset tcrasset force-pushed the tc/improve-testing branch from c464b60 to 67b9f8c Compare August 15, 2024 11:53
package.json Outdated Show resolved Hide resolved
src/app-sync.js Outdated
Comment on lines 16 to 17
app.use(express.json());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The /delete-user-file req.body was not being parsed as json by default, which necessitates the middleware to be added.

I manually tested that the /sync endpoint still works by launching the frontend add doing some syncable stuff, and watching the requests/responses.
It would be nice if there were tests for this function, which is my next plan.

@tcrasset tcrasset changed the title [WIP] improve testing utils and add delete-user-file test [Maintenance] improve testing utils and add delete-user-file test Aug 15, 2024
package.json Outdated Show resolved Hide resolved
src/app-sync.test.js Show resolved Hide resolved
Co-authored-by: Matt Fiddaman <[email protected]>
@tcrasset tcrasset requested a review from matt-fidd August 15, 2024 15:36
@tcrasset
Copy link
Contributor Author

Took your comments into account.

Also, during some more integration testing for /sync, I noticed that I actually needed the middleware to be express.raw for the /sync endpoint to work during integration tests. I would wager that this is specific to jest or supertest, as I had no issues without it when using the frontend. Same for the delete-user-file endpoint, worked prefectly using curl without specifying the express.json() middleware, but during integration testing it complained the body was empty, as specified in one of my previous comments.

Am not knowledgeable enough in javascript testing to know why that might be.

@tcrasset
Copy link
Contributor Author

Follow up PR is here: #423

@matt-fidd matt-fidd merged commit baf04a4 into actualbudget:master Aug 15, 2024
7 checks passed
MMichotte pushed a commit to MMichotte/actual-server that referenced this pull request Sep 9, 2024
…tualbudget#421)

* improve testing utils and add delete-user-file test

* remove linting errors

* add release notes

* match npm scripts naming style

Co-authored-by: Matt Fiddaman <[email protected]>

* add raw middleware for /sync

---------

Co-authored-by: Matt Fiddaman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants