add option for video-import CLI to wait between two video imports

[fflorent]

Description

• introduce --wait-interval <seconds> to wait <seconds> between 2 video imports (and prevent TOO_MANY_REQUESTS error from Youtube)
• some code style improvements (in a separate commit)

Related issues

Fixes #3309

Has this been tested?

• 👍 yes, I added tests to the test suite
• 👍 yes, light tests as follows are enough
• 💭 no, because this PR is a draft and still needs work
• 🙅 no, because they aren't needed
• 🙋 no, because I need help

[fflorent]

I see some errors on the CI related to files I haven't modified.

[rigelk]

@fflorent you filled the template erroneously, since adding options to the CLI requires tests be added in

PeerTube/server/tests/cli/create-import-video-file-job.ts

Line 33 in a158715

describe('Test create import video jobs', function () {

[fflorent]

@fflorent you filled the template erroneously, since adding options to the CLI requires tests be added in

PeerTube/server/tests/cli/create-import-video-file-job.ts

Line 33 in a158715

describe('Test create import video jobs', function () {

Alright, I'll add test and ask for help if I need some!

[rigelk]

@fflorent did you make any progress with your tests?

[fflorent]

Aie, I have to put a reminder in my agenda, as I am a bit busy.

I think I will make some progress during this month.

Florent

[rigelk]

@fflorent ping 🙂 could you also rebase and fix the lint errors?

[fflorent]

The rebase is done!

@fflorent you filled the template erroneously, since adding options to the CLI requires tests be added in

PeerTube/server/tests/cli/create-import-video-file-job.ts

Line 33 in a158715

describe('Test create import video jobs', function () {

The test file you mention is not the good one if I am not wrong. This file is for the creation of import file jobs (i.e. local file to import in PeerTube). This script is for importing videos using youtube-dl.

I am not sure if we have tests for this script (we could have, but I guess this would rely on mocks using proxyquire and sinon).

[rigelk]

@fflorent sorry for the delay in answering your question! You are right, that is not the right place to put your tests in. That would be in https://github.com/Chocobozzz/PeerTube//blob/8e76aa1d75aebdadd0451d2e57c9bb65d1e75b9a/server/tests/cli/peertube.ts#L191

[fflorent]

@rigelk Thanks for your reply.

I am sorry, I feel very uncomfortable with these tests especially for this option. Here are some points of why:

• The tests are tight together: the success of one of these tests is depending of the good execution of the preceding one, and there is no cleanup after each of them AFAIK (they are after the execution of the whole test file);
• A video of the youtube channel is already imported, the test I need to run needs the importation of two videos, thus I would need another channel with two videos or a way to undo the upload of the previous videos;
• The --wait-interval option is, IMO, not important enough for an integration test which are hard to implement and slow to run. A unit test would have been better for this, in which we could mock the dependencies of the script module and fake the timers. But AFAICT, there is no unit test for this command;

Sorry for being negative here. I don't blame anyone. I admire the work of all the contributors and the time spent on developing PeerTube. I am really grateful (:heart:), and wish to help. I miss time for that.

I'd rather close the PR if you don't mind. If you need to discuss a bit further, don't hesitate through IRC or Matrix on the PeerTube channel.

Cheers!

[rigelk]

* The `--wait-interval` option is, IMO, not important enough for an integration test which are hard to implement and slow to run. A unit test would have been better for this, in which we could mock the dependencies of the script module and [fake the timers](https://github.com/sinonjs/fake-timers). But AFAICT, there is no unit test for this command;

I agree here. Considering its ROI, we might as well just skip this test for now.

Sorry for being negative here. I don't blame anyone.

No worries.

@fflorent you marked the PR as a draft. Do you mind explaining what is yet to be done for this PR in your opinion?

[fflorent]

@rigelk I see three options for the remaining work to be done:

• either the integration tests are necessary for you to integrate that and the tests have to be written;
• either we agree that a unit test is missing and we can write one of them (this would require some dependencies like sinon and proxyquire to be added as dev-dependencies);
• either tests are considered as not necessary, and I guess we could merge the request just after a rebase;

If you agree with the second option and can wait a bit more, I can try in some weeks to make that (I'd do my best to invest time for that).

What do you think?

[rigelk]

@fflorent IMHO we can settle on option 3. I'll make a code review, and leave @Chocobozzz to evaluate options.

[fflorent]

IMHO we can settle on option 3. I'll make a code review, and leave @Chocobozzz to evaluate options.

Alright, I am ready for your remarks then :)

[Chocobozzz]

It's okay to not test this option