From 7f2ec1e802c063eda0f02bd4f99d6723a772f261 Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Fri, 10 Jan 2025 15:57:03 -0800 Subject: [PATCH] Added new sleep setting to yt-dlp runner and added tests --- lib/pinchflat/settings/setting.ex | 1 + lib/pinchflat/sources/sources.ex | 5 +- lib/pinchflat/utils/number_utils.ex | 14 +++ lib/pinchflat/yt_dlp/command_runner.ex | 24 ++++- test/pinchflat/settings_test.exs | 8 ++ test/pinchflat/sources_test.exs | 89 +++++++++++++++---- test/pinchflat/utils/number_utils_test.exs | 17 ++++ test/pinchflat/yt_dlp/command_runner_test.exs | 31 +++++++ 8 files changed, 165 insertions(+), 24 deletions(-) diff --git a/lib/pinchflat/settings/setting.ex b/lib/pinchflat/settings/setting.ex index b2cdb44..be1e596 100644 --- a/lib/pinchflat/settings/setting.ex +++ b/lib/pinchflat/settings/setting.ex @@ -45,5 +45,6 @@ defmodule Pinchflat.Settings.Setting do setting |> cast(attrs, @allowed_fields) |> validate_required(@required_fields) + |> validate_number(:extractor_sleep_interval, greater_than: 0) end end diff --git a/lib/pinchflat/sources/sources.ex b/lib/pinchflat/sources/sources.ex index ecbe7d4..99b4a56 100644 --- a/lib/pinchflat/sources/sources.ex +++ b/lib/pinchflat/sources/sources.ex @@ -180,9 +180,12 @@ defmodule Pinchflat.Sources do end defp add_source_details_to_changeset(source, changeset) do + original_url = changeset.changes.original_url use_cookies = Ecto.Changeset.get_field(changeset, :use_cookies) + # Skipping sleep interval since this is UI blocking and we want to keep this as fast as possible + addl_opts = [use_cookies: use_cookies, skip_sleep_interval: true] - case MediaCollection.get_source_details(changeset.changes.original_url, [], use_cookies: use_cookies) do + case MediaCollection.get_source_details(original_url, [], addl_opts) do {:ok, source_details} -> add_source_details_by_collection_type(source, changeset, source_details) diff --git a/lib/pinchflat/utils/number_utils.ex b/lib/pinchflat/utils/number_utils.ex index b7128f8..d86002b 100644 --- a/lib/pinchflat/utils/number_utils.ex +++ b/lib/pinchflat/utils/number_utils.ex @@ -36,4 +36,18 @@ defmodule Pinchflat.Utils.NumberUtils do end end) end + + @doc """ + Adds jitter to a number based on a percentage. Returns 0 if the number is less than or equal to 0. + + Returns integer() + """ + def add_jitter(num, jitter_percentage \\ 0.5) + def add_jitter(num, _jitter_percentage) when num <= 0, do: 0 + + def add_jitter(num, jitter_percentage) do + jitter = :rand.uniform(round(num * jitter_percentage)) + + round(num + jitter) + end end diff --git a/lib/pinchflat/yt_dlp/command_runner.ex b/lib/pinchflat/yt_dlp/command_runner.ex index 8391b5c..4e9e279 100644 --- a/lib/pinchflat/yt_dlp/command_runner.ex +++ b/lib/pinchflat/yt_dlp/command_runner.ex @@ -5,7 +5,9 @@ defmodule Pinchflat.YtDlp.CommandRunner do require Logger + alias Pinchflat.Settings alias Pinchflat.Utils.CliUtils + alias Pinchflat.Utils.NumberUtils alias Pinchflat.YtDlp.YtDlpCommandRunner alias Pinchflat.Utils.FilesystemUtils, as: FSUtils @@ -22,23 +24,23 @@ defmodule Pinchflat.YtDlp.CommandRunner do for a file watcher. - :use_cookies - if true, will add a cookie file to the command options. Will not attach a cookie file if the user hasn't set one up. + - :skip_sleep_interval - if true, will not add the sleep interval options to the command. + Usually only used for commands that would be UI-blocking Returns {:ok, binary()} | {:error, output, status}. """ @impl YtDlpCommandRunner def run(url, action_name, command_opts, output_template, addl_opts \\ []) do Logger.debug("Running yt-dlp command for action: #{action_name}") - # This approach lets us mock the command for testing - command = backend_executable() output_filepath = generate_output_filepath(addl_opts) print_to_file_opts = [{:print_to_file, output_template}, output_filepath] - user_configured_opts = cookie_file_options(addl_opts) + user_configured_opts = cookie_file_options(addl_opts) ++ sleep_interval_opts(addl_opts) # These must stay in exactly this order, hence why I'm giving it its own variable. all_opts = command_opts ++ print_to_file_opts ++ user_configured_opts ++ global_options() formatted_command_opts = [url] ++ CliUtils.parse_options(all_opts) - case CliUtils.wrap_cmd(command, formatted_command_opts, stderr_to_stdout: true) do + case CliUtils.wrap_cmd(backend_executable(), formatted_command_opts, stderr_to_stdout: true) do # yt-dlp exit codes: # 0 = Everything is successful # 100 = yt-dlp must restart for update to complete @@ -96,6 +98,20 @@ defmodule Pinchflat.YtDlp.CommandRunner do end end + defp sleep_interval_opts(addl_opts) do + sleep_interval = Settings.get!(:extractor_sleep_interval) + + if sleep_interval <= 0 || Keyword.get(addl_opts, :skip_sleep_interval) do + [] + else + [ + sleep_requests: NumberUtils.add_jitter(sleep_interval), + sleep_interval: NumberUtils.add_jitter(sleep_interval), + sleep_subtitles: NumberUtils.add_jitter(sleep_interval) + ] + end + end + defp add_cookie_file do base_dir = Application.get_env(:pinchflat, :extras_directory) filename_options_map = %{cookies: "cookies.txt"} diff --git a/test/pinchflat/settings_test.exs b/test/pinchflat/settings_test.exs index 944eaf1..f6117cf 100644 --- a/test/pinchflat/settings_test.exs +++ b/test/pinchflat/settings_test.exs @@ -77,5 +77,13 @@ defmodule Pinchflat.SettingsTest do assert %Ecto.Changeset{} = Settings.change_setting(setting, %{onboarding: true}) end + + test "ensures the extractor sleep interval is positive" do + setting = Settings.record() + + assert %Ecto.Changeset{valid?: true} = Settings.change_setting(setting, %{extractor_sleep_interval: 1}) + assert %Ecto.Changeset{valid?: true} = Settings.change_setting(setting, %{extractor_sleep_interval: 0}) + assert %Ecto.Changeset{valid?: false} = Settings.change_setting(setting, %{extractor_sleep_interval: -1}) + end end end diff --git a/test/pinchflat/sources_test.exs b/test/pinchflat/sources_test.exs index e72c05c..766c71f 100644 --- a/test/pinchflat/sources_test.exs +++ b/test/pinchflat/sources_test.exs @@ -354,7 +354,56 @@ defmodule Pinchflat.SourcesTest do end end - describe "create_source/2 when testing options" do + describe "create_source/2 when testing yt-dlp options" do + test "sets use_cookies to true if the source has been set to use cookies" do + expect(YtDlpRunnerMock, :run, fn _url, :get_source_details, _opts, _ot, addl -> + assert Keyword.get(addl, :use_cookies) + + {:ok, playlist_return()} + end) + + valid_attrs = %{ + media_profile_id: media_profile_fixture().id, + original_url: "https://www.youtube.com/channel/abc123", + use_cookies: true + } + + assert {:ok, %Source{}} = Sources.create_source(valid_attrs) + end + + test "does not set use_cookies to false if the source has not been set to use cookies" do + expect(YtDlpRunnerMock, :run, fn _url, :get_source_details, _opts, _ot, addl -> + refute Keyword.get(addl, :use_cookies) + + {:ok, playlist_return()} + end) + + valid_attrs = %{ + media_profile_id: media_profile_fixture().id, + original_url: "https://www.youtube.com/channel/abc123", + use_cookies: false + } + + assert {:ok, %Source{}} = Sources.create_source(valid_attrs) + end + + test "skips sleep interval" do + expect(YtDlpRunnerMock, :run, fn _url, :get_source_details, _opts, _ot, addl -> + assert Keyword.get(addl, :skip_sleep_interval) + + {:ok, playlist_return()} + end) + + valid_attrs = %{ + media_profile_id: media_profile_fixture().id, + original_url: "https://www.youtube.com/channel/abc123" + } + + assert {:ok, %Source{}} = Sources.create_source(valid_attrs) + end + end + + describe "create_source/2 when testing its options" do test "run_post_commit_tasks: false won't enqueue post-commit tasks" do expect(YtDlpRunnerMock, :run, &channel_mock/5) @@ -902,28 +951,30 @@ defmodule Pinchflat.SourcesTest do end defp playlist_mock(_url, :get_source_details, _opts, _ot, _addl) do - { - :ok, - Phoenix.json_library().encode!(%{ - channel: nil, - channel_id: nil, - playlist_id: "some_playlist_id_#{:rand.uniform(1_000_000)}", - playlist_title: "some playlist name" - }) - } + {:ok, playlist_return()} end defp channel_mock(_url, :get_source_details, _opts, _ot, _addl) do + {:ok, channel_return()} + end + + defp playlist_return do + Phoenix.json_library().encode!(%{ + channel: nil, + channel_id: nil, + playlist_id: "some_playlist_id_#{:rand.uniform(1_000_000)}", + playlist_title: "some playlist name" + }) + end + + defp channel_return do channel_id = "some_channel_id_#{:rand.uniform(1_000_000)}" - { - :ok, - Phoenix.json_library().encode!(%{ - channel: "some channel name", - channel_id: channel_id, - playlist_id: channel_id, - playlist_title: "some channel name - videos" - }) - } + Phoenix.json_library().encode!(%{ + channel: "some channel name", + channel_id: channel_id, + playlist_id: channel_id, + playlist_title: "some channel name - videos" + }) end end diff --git a/test/pinchflat/utils/number_utils_test.exs b/test/pinchflat/utils/number_utils_test.exs index db17e1c..1eae570 100644 --- a/test/pinchflat/utils/number_utils_test.exs +++ b/test/pinchflat/utils/number_utils_test.exs @@ -47,4 +47,21 @@ defmodule Pinchflat.Utils.NumberUtilsTest do assert NumberUtils.human_byte_size(nil) == {0, "B"} end end + + describe "add_jitter/2" do + test "returns 0 when the number is less than or equal to 0" do + assert NumberUtils.add_jitter(0) == 0 + assert NumberUtils.add_jitter(-1) == 0 + end + + test "returns the number with jitter added" do + assert NumberUtils.add_jitter(100) in 100..150 + end + + test "optionally takes a jitter percentage" do + assert NumberUtils.add_jitter(100, 0.1) in 90..110 + assert NumberUtils.add_jitter(100, 0.5) in 50..150 + assert NumberUtils.add_jitter(100, 1) in 0..200 + end + end end diff --git a/test/pinchflat/yt_dlp/command_runner_test.exs b/test/pinchflat/yt_dlp/command_runner_test.exs index 898e64b..61a91c6 100644 --- a/test/pinchflat/yt_dlp/command_runner_test.exs +++ b/test/pinchflat/yt_dlp/command_runner_test.exs @@ -1,6 +1,7 @@ defmodule Pinchflat.YtDlp.CommandRunnerTest do use Pinchflat.DataCase + alias Pinchflat.Settings alias Pinchflat.Utils.FilesystemUtils alias Pinchflat.YtDlp.CommandRunner, as: Runner @@ -95,6 +96,36 @@ defmodule Pinchflat.YtDlp.CommandRunnerTest do end end + describe "run/4 when testing sleep interval options" do + test "includes sleep interval options by default" do + Settings.set(extractor_sleep_interval: 5) + + assert {:ok, output} = Runner.run(@media_url, :foo, [], "") + + assert String.contains?(output, "--sleep-interval") + assert String.contains?(output, "--sleep-requests") + assert String.contains?(output, "--sleep-subtitles") + end + + test "doesn't include sleep interval options when skip_sleep_interval is true" do + assert {:ok, output} = Runner.run(@media_url, :foo, [], "", skip_sleep_interval: true) + + refute String.contains?(output, "--sleep-interval") + refute String.contains?(output, "--sleep-requests") + refute String.contains?(output, "--sleep-subtitles") + end + + test "doesn't include sleep interval options when extractor_sleep_interval is 0" do + Settings.set(extractor_sleep_interval: 0) + + assert {:ok, output} = Runner.run(@media_url, :foo, [], "") + + refute String.contains?(output, "--sleep-interval") + refute String.contains?(output, "--sleep-requests") + refute String.contains?(output, "--sleep-subtitles") + end + end + describe "run/4 when testing global options" do test "creates windows-safe filenames" do assert {:ok, output} = Runner.run(@media_url, :foo, [], "")