Removes shelljs#8366
Conversation
|
TODO:
|
|
Nice idea! Removing |
|
How are you planning to test this? I think we can just run the bots and see what it does, or are changes on the bots required for this as well? |
| if (errors) { | ||
| echo('Found ' + errors + ' expectation failures.'); | ||
| console.error('Found ' + errors + ' expectation failures.'); | ||
| process.exit(1); |
There was a problem hiding this comment.
Shouldn't external/builder/test-fixtures.js have this as well, since they're quite similar? The same holds for the else branch here.
| }; | ||
| cmdCheckAllKilled = { | ||
| file: 'wmic', | ||
| args: wmicPrefix.concat(['call', 'get CommandLine']) |
There was a problem hiding this comment.
I think 'call' should be removed here, i.e., it should probably be wmicPrefix.concat(['get', 'CommandLine']) to match the other one.
I'm not sure what sort of testing you had in mind here, however: |
Mostly aborting hanging browser process. |
I've just tested this (on Windows), by reverting a patch (on top of this PR) that fixed an infinite loop, and once the timeout[1] was reached the browser was closed/cleaned-up properly. [1] Line 100 in 658fb03 [2] I'm assuming that issues with aborting a hanging browser process would have prevented a new test from being successfully triggered. |
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 |
From: Bot.io (Linux m4)SuccessTotal script time: 24.97 mins
|
From: Bot.io (Windows)SuccessTotal script time: 25.64 mins
|
There was a problem hiding this comment.
You should be able to remove lines test/.eslintrc#L8 and external/.eslintrc#L8 too, since there shouldn't be any code left relying on ShellJS.
Edit: Apparently that causes linting failures in https://github.com/mozilla/pdf.js/tree/master/external/crlfchecker. However, that entire folder appears to be completely unused now, so let's just remove it :-)
This looks good to me, thanks for cleaning this up!
@yurydelendik Unless there's any more tests that you think we need to run here, please feel free to land this!
Removes shelljs
4 participants
No description provided.