Don't write an empty PDF file when downloading of a linked test file fails

[Snuffleupagus]

Currently test/downloadutils.js will write out an empty file when downloading of a linked file fails. The consequence of this is that since the PDF file now "exists", no further attempts to download it will be made unless the empty PDF is removed.
This is especially annoying when it happens on the bots, since it requires that someone logs in and manually removes the empty PDF file.

I'm really not sure why it was implemented this way to begin with. However, given that PDFJS.getDocument fails with MissingPDFException if a file is not present, I cannot see a problem with this change.
Furthermore, the patch also changes the checkRefTestResults in test/test.js such that we treat the "downloading of linked test file failed" case as a failure, so that e.g. the bots won't report "success" when a test file failed to download.

[yurydelendik]

Given a nature of linked files, saving empty and error file was done to allow beginner contributor to at least continue with some subset of test. I would like to preserve this behavior in someor even relaxed form. We don't want to discourage people to use testing. For the server we can enforce failures for non-downloaded files.

[yurydelendik]

This is especially annoying when it happens on the bots, since it requires that someone logs in and manually removes the empty PDF file.

We need to disable these line for 'test'/'unittest' and keep it only for 'makeref'.

https://github.com/mozilla/botio-files-pdfjs/blob/master/on_cmd_test.js#L80-L86

[Snuffleupagus]

Given a nature of linked files, saving empty and error file was done to allow beginner contributor to at least continue with some subset of test. I would like to preserve this behavior in someor even relaxed form. We don't want to discourage people to use testing. For the server we can enforce failures for non-downloaded files.

I can certainly revert the changes to test/test.js, but I still don't see the point of writing an empty file.
Am I missing obvious something here?

Consider the way that things currently work, when one of these empty files are loaded it will trigger this assert: https://github.com/mozilla/pdf.js/blob/master/src/core/document.js#L385; and the loadingTask will thus be rejected with UnknownErrorException.
With this patch, the loadingTask will instead be rejected with MissingPDFException.

So, without the change to test/test.js, we'd still get similar behavior here but with the added bonus of not being perpetually stuck with an empty test file (this ought to be helpful not just on the bots, but for people running tests locally as well)!

[yurydelendik]

I can certainly revert the changes to test/test.js, but I still don't see the point of writing an empty file.
Am I missing obvious something here?

In the past (test.py) it was done to suppress further attempts to download with subsequent test.py run, e.g. if you had multiple pdfs files failed to download, you could get several minutes of retry until testing will run.

[yurydelendik]

Can we split download of pdf corpus into separate task? If pdf were not downloaded it will just skip them with warning during testing. Download task will never create an error/empty file, and every time it will try to download missing pdfs and erroring if at least one fails.

[Snuffleupagus]

Can we split download of pdf corpus into separate task? If pdf were not downloaded it will just skip them with warning during testing. Download task will never create an error/empty file, and every time it will try to download missing pdfs and erroring if at least one fails.

Yes, that definitely sounds like the best solution here!
Since this is something that I'm not really affected by myself, I probably won't work on it though (at least not for the foreseeable future, since my time is a bit limited).