Testsuite improvements #3555
Testsuite improvements #3555
Conversation
daneryl
force-pushed
the
testsuite_improvements
branch
3 times, most recently
from
5de9253 to
8a1403a
Compare
daneryl
force-pushed
the
testsuite_improvements
branch
from
8a1403a to
b20da8b
Compare
There was a problem hiding this comment.
I think these are great improvements. Just a couple of comments / questions more than changes request. I ran the suite several times, I had the exportRoutes timeout in one of the runs, but could not replicate it again, so it may have been an intermittent test or something just hanging up on my end.
app/api/files/filesystem.ts
Outdated
| uploadedDocuments: `${__dirname}/specs/uploads/`, | ||
| attachments: `${__dirname}/specs/uploads/`, | ||
| customUploads: `${__dirname}/specs/customUploads/`, | ||
| customUploads: `${__dirname}/specs/uploads/`, | ||
| temporalFiles: `${__dirname}/specs/uploads/`, |
There was a problem hiding this comment.
I think this is potentially dangerous. Having all test paths be identical means that you could 'use' the wrong path or interchange the paths in the tests and they will pass nonetheless. I believe that was the intent of having 'customUploads' be a different path. Should we create a tech debt issue with this? Or do you think it is not worth it?
There was a problem hiding this comment.
@RafaPolit i have reverted the customUploads path, its now /specs/customUploads again, for the others i think the issue described still remains, we should create a tech debt.
| @@ -26,8 +27,9 @@ describe('upload routes', () => { | |||
| let req; | |||
| let file; | |||
|
|
|||
| const deleteAllFiles = cb => { | |||
| const directory = `${__dirname}/uploads/`; | |||
| const deleteAllFiles = async cb => { | |||
There was a problem hiding this comment.
There are more calls to deleteAllFiles in this spec that are not being awaited. They have a done inside that could cause the same effect, but maybe for consistency add the await on those calls? (currently lines 88 and 186)
There was a problem hiding this comment.
Good catch, the 2 calls are now properly awaited
app/api/files/filesystem.ts
Outdated
| uploadedDocuments: `${__dirname}/specs/uploads/${subPath}`, | ||
| attachments: `${__dirname}/specs/uploads/${subPath}`, | ||
| customUploads: `${__dirname}/specs/uploads/${subPath}`, | ||
| temporalFiles: `${__dirname}/specs/uploads/${subPath}`, |
There was a problem hiding this comment.
Same concern as above,! Maybe this solves it if we are using only one at a time in each test?
There was a problem hiding this comment.
this was not being used anymore actually, it has been removed now.
daneryl
force-pushed
the
testsuite_improvements
branch
2 times, most recently
from
7df7ec9 to
d6a3528
Compare
|
LGTM |
fixes: #3561
PR checklist:
QA checklist: