Drop obsolete logic from the downloadFile function in test/downloadutils.js
#13420
Conversation
…dutils.js` This code is old and predates the improvements we made to the test manifest to only contain working URLs (either Web Archive or GitHub/Bugzilla links), so the fallback logic to try the Web Archive is no longer necessary. This greatly simplifies the function and also makes sure that we fail directly in case a bad URL is added to the manifest, instead of having it work "accidentally" because of this logic, since we want the manifest to be correct at all times (and otherwise fail loudly).
timvandermeij
force-pushed
the
test-downloadutils
branch
from
330e018 to
9943022
Compare
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/fdd6de0de2181ad/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://3.101.106.178:8877/160c21616820348/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/fdd6de0de2181ad/output.txt Total script time: 26.18 mins
Image differences available at: http://54.67.70.0:8877/fdd6de0de2181ad/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/160c21616820348/output.txt Total script time: 29.41 mins
Image differences available at: http://3.101.106.178:8877/160c21616820348/reftest-analyzer.html#web=eq.log |
There was a problem hiding this comment.
This greatly simplifies the function and also makes sure that we fail directly in case a bad URL is added to the manifest,
Failing immediately/explicitly is much nicer indeed, since there's nothing worse than a silent failure when trying to debug something!
instead of having it work "accidentally" because of this logic, since we want the manifest to be correct at all times (and otherwise fail loudly).
Honestly, the fallback logic was probably never all that helpful to begin with, since it's likely that the md5-hashes didn't match and in that case you might not even test the intended thing anyway.
Looks good to me, whatever can be done to modernize/improve the test-code is a welcome change; thank you!
This patch is a first step to refactor the testing code to make it more manageable. This is needed in order to, over time, enable the
no-varrule there too, which is now made difficult by lots of global variable and callback usage.This code is old and predates the improvements we made to the test manifest to only contain working URLs (either Web Archive or GitHub/Bugzilla links), so the fallback logic to try the Web Archive is no longer necessary. This greatly simplifies the function and also makes sure that we fail directly in case a bad URL is added to the manifest, instead of having it work "accidentally" because of this logic, since we want the manifest to be correct at all times (and otherwise fail loudly).
To verify that this works I removed my local
test/pdfsfolder, did a clean checkout of it and ran the tests, which forced a re-download of all files. All files downloaded correctly, there were no MD5 checksum mismatches and the files could also be opened correctly.