Fix: Blacklist redirect #118
Conversation
|
@erikkemperman I'm sorry my modification and About the unit test of logLevel, using About I wish there are better alternatives... |
|
Maybe "annoying" was the wrong word, let's make it "complicated". I wanted to submit a really simple PR but got stuck on all these failing tests :-) If you agree this solves things at least for the time being, could you review and merge? Thanks! |
|
@erikkemperman I've reviewed this PR and it's good. |
|
@sttk Thanks for review and merge! Unfortunately my day is almost over, so my simple little PR will have to wait a bit. |
|
@erikkemperman sorry this tripped you up. Testing CLI tools is quite tricky in node, but hopefully we can make incremental improvements over time. Also note that the SSL change was sprung on me and I didn't have a chance find everything that would be affected. Thanks for fixing this; I'll get it shipped in a patch. |
|
Looks like we have some features and updates queued up so this will go out as 1.4.0 |
Several tests were failing for me here, while the exact same code on gulp/master was somehow fine.
Superficially the errors seemed to be about flags, such as loglevel... But it was something else entirely.
Turns out an external change sabotaged a bunch of our tests here, viz. we now get a "moved permanently" (301) when fetching the blacklist, and apparently the wreck library doesn't follow redirects (by default).
I fixed the URL in this PR, perhaps the wreck options should be revisited some time.
This was pretty annoying to track down... The tests are already slightly tricky to debug because stdout and stderr are hijacked. But it becomes really problematic if a unit test depends on unrelated components, both internal (in this case, the getBlacklist module) and external (the corresponding webserver).
IMHO, it should not be possible for this kind of external change to break unit tests?