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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add clean deleted responses task on startup, and add tests #1292

Merged
merged 5 commits into from Dec 5, 2018

Conversation

Projects
None yet
2 participants
@rickychandra
Copy link
Contributor

rickychandra commented Dec 3, 2018

Closes #1201
I add a function in models.response that will get all response files from responses directory. Then check whether each of them exists in the db. If no, then delete the file.
I batch each checks to the db by MAX_RESPONSES (currently 20) responses, on the assumption that there could be many files in the responses directory to be checked.
I also add tests for this function, but reasons I don't know yet, the jest.useFakeTimers() used in the migrate() test suite in the same file causes the subsequent suites' tests to timeout when they are calling async/await functions. Adding jest.useRealTimers() seems to solve it 馃槃.

@gschier
Copy link
Collaborator

gschier left a comment

Thanks for submitting this @rickychandra! Looks great, just a few comments.


// ~~ is for division without remainder.
const totalBatch = ~~((files.length - 1) / MAX_RESPONSES) + 1;
for (let batch = 0; batch < totalBatch; batch++) {

This comment has been minimized.

@gschier

gschier Dec 3, 2018

Collaborator

Is there a reason you're doing these in batches? It's difficult for me to tell what's going on.

Since we're using an in-memory database, it should be performant enough to loop over each response file one-by-one. Either that or fetch all responses from the DB at once, in one query.

This comment has been minimized.

@rickychandra

rickychandra Dec 4, 2018

Contributor

Actually I have concern that loading all responses will take too much memory.
I did a little benchmark on the 3 approaches (fetch for each bodyPath from db, batch fetch, and fetch all from db) using non in-memory db with 2000 responses (~100 requests) in db and 4000 from filesystem.

  • The fetch-for-each is the slowest, on average 10.5 secs.
  • The batch-fetch on average 1.6 sec.
  • The fetch-all is the fastest, on average 0.5 sec. But I go with this option now, because I don't see any significant memory usages when I ran the performance profiler in the DevTools. Or maybe we could wait until there is a report on this 馃槵.

This comment has been minimized.

@rickychandra

rickychandra Dec 4, 2018

Contributor

I ran the benchmark with the spec:

Linux Ubuntu 18.04
Memory 15.5 GiB
OS type x64
Intel Core i5-6200U CPU @ 2.30GHz x 4
@@ -106,6 +106,8 @@ export async function init() {

_writeChangesInterval = setInterval(writePendingChanges, WRITE_PERIOD);

await models.response.cleanDeletedResponses();

This comment has been minimized.

@gschier

gschier Dec 3, 2018

Collaborator

This isn't a great place to put this since it doesn't have anything to do with the sync system. However, there was no other option so I don't blame you 馃槃

I just pushed a commit to the develop branch to support an option hookDatabaseInit() in each model that will be called when the DB is initially created. Please move this call to there:

export async function hookDatabaseInit() {
console.log('Init responses DB');
}

This comment has been minimized.

@rickychandra

rickychandra Dec 4, 2018

Contributor

Sure!
Actually until just now I thought package sync means synchronous, instead of synchronize. Aside from that I also thought it was a place for random operations (it says "things that can wait" here) 馃槅.

@@ -134,3 +139,80 @@ describe('migrate()', () => {
).toBe('zip');
});
});

describe('cleanDeletedResponses()', function() {

This comment has been minimized.

@gschier

gschier Dec 3, 2018

Collaborator

Nice work on these tests 馃憤

This comment has been minimized.

@rickychandra

rickychandra Dec 4, 2018

Contributor

Thanks! This is my first time working with Jest and mocking functions seems to have many gotchas, such as affecting other test suites. I spent a few hours debugging them 馃槅.

This comment has been minimized.

@gschier

gschier Dec 5, 2018

Collaborator

Ya, it can be tricky sometimes for sure. I'm quite the fan of Jest though as far as JS test frameworks go 馃挴

gschier and others added some commits Nov 30, 2018

@gschier

gschier approved these changes Dec 5, 2018

Copy link
Collaborator

gschier left a comment

Awesome stuff! Going to merge this in now 馃憤 馃弲

@@ -236,3 +238,24 @@ function migrateBodyCompression(doc: Object) {

return doc;
}

export async function cleanDeletedResponses() {

This comment has been minimized.

@gschier

gschier Dec 5, 2018

Collaborator

Beautiful. Super clean and simple now 馃憤 馃槃

@@ -134,3 +139,80 @@ describe('migrate()', () => {
).toBe('zip');
});
});

describe('cleanDeletedResponses()', function() {

This comment has been minimized.

@gschier

gschier Dec 5, 2018

Collaborator

Ya, it can be tricky sometimes for sure. I'm quite the fan of Jest though as far as JS test frameworks go 馃挴

@gschier gschier merged commit f88d961 into getinsomnia:develop Dec 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rickychandra rickychandra deleted the rickychandra:feature/clean-responses-task branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment