GSoC 2021 : Unit Tests for MRI and Upload Controllers added and Istanbul script updated....

[adityaofficial10]

What additions have been made?

• Istanbul NYC configuration file added.
• Istanbul script updated..
• Minor additions made in the MRI controller.
• Unit tests added for MRI controller.
• Unit tests added for Upload controller.

This would be a crucial addition to our unit test suite as we have shot up the coverage of MRI module by 25%!

I have left upload function from Upload Controller as it would be incorporated later. @katjaq @r03ert0 Kindly have a look.

[adityaofficial10]

thank you @adityaofficial10 !

• would it be possible to keep the name of the test files consistent with that of the file they test? Also, use .spec instead of .test, to be coherent with our other project Microdraw. So, for example, instead of mriController.test.js it should be mri.controller.spec.js
• would it be possible to have a separate .spec.js file for each .js file in the unit tests? For example, the tests for the upload controller should go in their own file called upload.controller.spec.js
• many let in the tests could be replaced with const because the variable value is not re-assigned. I thought that was checked by the eslint script. If that's not the case, it should be...
• you are using files from the ABIDE project (hosted in amazon s3) instead of the local test data. Why? It's not wrong, but I don't know if we are not testing just that the connection works ok. Any thoughts on this @ntraut @katjaq ?

@r03ert0 I've made the suggested changes and the test coverage has also gone up by 20% in the MRI controller. Also, I don't know how but two integration tests are failing somehow. And there are some code scanning errors which I can't understand.

[adityaofficial10]

Hey @ntraut ! I don't why build is failing here. I examined the tests and I saw that 2 of the tests were failing. Can you help me here?

[r03ert0]

hello @adityaofficial10 and @ntraut ! To summarise what I described in our Mattermost channel:

The buffer too small error comes from an error triggered by the downloadMRI polling system. Additionally, the apiMriPost test should be split in two. Here's what should be done:

• use the correct URL in the apiMriPost test (https://s3.amazon...) • make sure that there's no ongoing download: the downloadQueue should be empty at the end of the test! • add the appropriate checks for properties such as req.headers in mri.controller.js • make a test for getting paginated output • make a test for incorrect URL • add URL validation in mri.controller.js(so that the previous test can fail :smile:)

[ntraut]

Hello @r03ert0, the test generating a buffer size error is this one POST /mri/json with url should start a download. The error happens on github actions but not in my computer, does it happen also on your side? The test seems to be launched twice on github actions, could not it be a merging problem? Maybe we should try to rebase this branch on master and see if the errors still happens?

There are also CodeQL but they seem to come from old code which just had been formatted on this PR.

[ntraut]

Hey @ntraut ! I don't why build is failing here. I examined the tests and I saw that 2 of the tests were failing. Can you help me here?

I don't see either, on my computer they are not failing so it's hard to debug them for me. Are they also failing on your side?

[r03ert0]

Hey @ntraut ! I don't why build is failing here. I examined the tests and I saw that 2 of the tests were failing. Can you help me here?

I don't see either, on my computer they are not failing so it's hard to debug them for me. Are they also failing on your side?

I know why those tests are failing. I described that more in detail in the mattermost. Briefly, it's a call to the download api that fails. Calls to the download api are answered immediately not to hang the browser. They trigger the download in the server. The client needs to keep asking for the file. The server will answer saying that dowload is still in progress, succeeded (in which case the file will be available) or failed. In the case of the test, the download failed (there's no mgz file at the url requested, only the json file associated to that file). Concretely, the test needs to be changed to deal with the download polling system correctly.

[ntraut]

Hey @ntraut ! I don't why build is failing here. I examined the tests and I saw that 2 of the tests were failing. Can you help me here?

I don't see either, on my computer they are not failing so it's hard to debug them for me. Are they also failing on your side?

I know why those tests are failing. I described that more in detail in the mattermost. Briefly, it's a call to the download api that fails. Calls to the download api are answered immediately not to hang the browser. They trigger the download in the server. The client needs to keep asking for the file. The server will answer saying that dowload is still in progress, succeeded (in which case the file will be available) or failed. In the case of the test, the download failed (there's no mgz file at the url requested, only the json file associated to that file). Concretely, the test needs to be changed to deal with the download polling system correctly.

So they are also failing on your computer? I don't know why they seem to pass on my computer, i looked at the log and it seemed normal, but on the log of github actions there is a strange thing like if the test "POST /mri/json with url should start a download" was passed twice simultaneously. Do you also have that problem on your side?

Anyway we will have to rebase that branch before merging, so we can start doing that and pray that it solves the errors miraculously...

[r03ert0]

Hey @ntraut ! I don't why build is failing here. I examined the tests and I saw that 2 of the tests were failing. Can you help me here?

I don't see either, on my computer they are not failing so it's hard to debug them for me. Are they also failing on your side?

I know why those tests are failing. I described that more in detail in the mattermost. Briefly, it's a call to the download api that fails. Calls to the download api are answered immediately not to hang the browser. They trigger the download in the server. The client needs to keep asking for the file. The server will answer saying that dowload is still in progress, succeeded (in which case the file will be available) or failed. In the case of the test, the download failed (there's no mgz file at the url requested, only the json file associated to that file). Concretely, the test needs to be changed to deal with the download polling system correctly.

So they are also failing on your computer? I don't know why they seem to pass on my computer, i looked at the log and it seemed normal, but on the log of github actions there is a strange thing like if the test "POST /mri/json with url should start a download" was passed twice simultaneously. Do you also have that problem on your side?

Anyway we will have to rebase that branch before merging, so we can start doing that and pray that it solves the errors miraculously...

the weird, tricky thing is that the test that triggers the download doesn't fail (because it does get an answer from the server). The error only shows up later, in a test that's completely unrelated. I imagine that it will depend on the speed of the computer running the tests where the fail will show up! (in my case it was in the next test file that was launched because of the alphabetical order of the filenames)

[ntraut]

Ok, sorry if it is not clear for me but that's probably because i can't reproduce the failure. Anyway the most important is that you and @adityaofficial10 can reproduce it. As this is an info that i can't find and to better identify the problem, what is the test causing the failure if it's not "POST /mri/json with url should start a download"?

[ntraut]

Ok, sorry if it is not clear for me but that's probably because i can't reproduce the failure. Anyway the most important is that you and @adityaofficial10 can reproduce it. As this is an info that i can't find and to better identify the problem, what is the test causing the failure if it's not "POST /mri/json with url should start a download"?

It is fixed now but for information the test which caused the failure was it('should work correctly and make the right calls when input is correct'). I was not able to reproduce the failure because of a path problem which interrupted the test before the end but the test was still reported to pass.

[ntraut]

hello, i rebased the branch on the current master. one test failed on github actions but i think it is only a timeout issue (which in my opinion we should think to increase them significantly to get rid of that kind of issues) is that ok for you to merge this pr? for me the test description issues are not a good reason not to merge because there are already that kind of issues in the previously merged pr and it can be fixed later.

[katjaq]

Thank you for all the feedback, testing, fixing, and best practice adaptations @ntraut ! This is really awesome! I agree, thank you for rebasing and updating after rebase. Let's merge before this dangles any longer.

Now that things are fresh, please feel free to add an issue(s) to keep track of things like the issue with the test descriptions, we could tag @adityaofficial10 and kindly ask to rework the descriptions based on your feedback and send them in a separate PR. Also please add an issue (or a PR 😇 ) for the increase of the timeout: as we had that several times already, we should increase it, but I wouldn't be able to recommend by how much. Fell free to take the decision there.

So, here, all green light to merge! Thank you so much to both of you @adityaofficial10 and @ntraut