Add test to invoke onigumo from CLI #159

nappex · 2023-01-15T13:23:23Z

We dont test invocation Onigumo from CLI at this moment.
We should add some basic test to test that commands $ ./onigumo or $ ./onigumo Downloader in terminal are running without an error.

Unfortunately I have no idea how should be tested... So the PR is just draft to discuss..

Glutexo · 2023-01-16T11:00:48Z

There are two options, not mutually exclusive:

An integration test that would run the actual Downloader, only with HTTPoison mocked.
A unit test that would test the CLI module but wouldn’t use the actual Downloader.

I think both are good to have. For the latter, we’d need to specify a Behaviour. That is not only a good learning opportunity but a correct thing to do too. The Downloader is a module with a public interface: the main function.

Glutexo · 2023-01-16T11:01:45Z

Now I see, however, that Behaviours are deprecated. I need to explore that more.

This module is deprecated. Use @callback and @macrocallback attributes instead.

It’s probably just a syntactic change in how Behaviours are defined, not a deprecation of the idea itself. More reading here: https://hexdocs.pm/elixir/typespecs.html#behaviours

Glutexo

The integration test will be almost identical to the existing “main” function test:

Prepare a list of URLs to download.
Invoke the Downloader.
Check the downloaded files.

A technical note: keep only the integration tests in this PR. For the future unit test, extract the behaviour introduction and the actual test into separate PRs.

test/onigumo_cli_test.exs

Glutexo · 2023-02-06T13:59:42Z

@nappex, is everything clear? Do you have all the information you need to complete this pull request? Can I provide any help?

Glutexo

If I recall correctly, you addressed this during our last meeting and had written an integration test for it, right? If that's the case, let's make sure to finalize it. We need that test in place. Let me know if you need any help or if there's anything I can do to assist you.

test/onigumo_cli_test.exs

- These functions was moved because we need them in more tests then just one

- when we move the file to folder support util in name is extra. The mean of util is include in folder name support

mix.exs

nappex · 2023-05-23T09:59:48Z

It is ready to review and merge.

There are two not ideal approaches:

we change cwd, because the root_path is set as constant and code does not see created files needed to further processes. For this case was issue created Fix tests with root_path #184 so it should be fix by another PR.
This PR does not test invalid inputs to CLI. There is an issue for that case Test CLI for invalid input #183

Glutexo

💥

Glutexo · 2023-05-22T14:03:00Z

mix.exs

@@ -8,7 +8,8 @@ defmodule Onigumo.MixProject do
      elixir: "~> 1.10",
      start_permanent: Mix.env() == :prod,
      deps: deps(),
-      escript: escript()
+      escript: escript(),
+      elixirc_paths: elixirc_paths(Mix.env()),


I am thinking about extracting Mix.env() to a variable to save two identical calls.

Glutexo · 2023-07-29T16:19:58Z

mix.exs

@@ -8,7 +8,8 @@ defmodule Onigumo.MixProject do
      elixir: "~> 1.10",
      start_permanent: Mix.env() == :prod,
      deps: deps(),
-      escript: escript()
+      escript: escript(),
+      elixirc_paths: elixirc_paths(Mix.env())


This was a nice rabbit hole to find out where elixir_rcpaths is defined. Noting it here for future generations:

mix test, as its first step, starts the application. That is, it runs mix run. (reference)

This task starts the current application, […]

mix run compiles the current application before starting it. That is, it runs mix compile. (reference)

Before doing anything, Mix will compile the current application […].

mix compile runs all compilers. Among them, mix compile.elixir. (reference)

It simply runs the compilers registered in your project […].
[…]
:compilers - compilers to run, defaults to Mix.compilers/0, which are [:yecc, :leex, :erlang, :elixir, :app].

mix compile.elixir uses the elixirc_paths to find source files. (reference)

:elixirc_paths - directories to find source files. Defaults to ["lib"].

Good point we should look at closer!

Glutexo · 2023-07-29T16:21:46Z

mix.exs

+  defp elixirc_paths(:test), do: ["lib", "test/support"]
+  defp elixirc_paths(_), do: ["lib"]


Could we define a common elixirc_path function that would return the default list of ["lib"] and only add test/support to that in case of :test? By explicitly mentioning "lib" here, we duplicate what’s already defined in Mix instead of adding to that.

test/onigumo_cli_test.exs

test/support/input.ex

test/support/http.ex

Glutexo · 2023-10-01T18:29:04Z

mix.exs

@@ -8,7 +8,8 @@ defmodule Onigumo.MixProject do
      elixir: "~> 1.10",
      start_permanent: Mix.env() == :prod,
      deps: deps(),
-      escript: escript()
+      escript: escript(),
+      elixirc_paths: elixirc_paths(Mix.env())


Mix.env() is called twice. We can save the repeated call by storing the returned value to a variable.

Add test to invoke onigumo from CLI

301fd25

Glutexo assigned nappex Jan 16, 2023

Glutexo self-requested a review January 16, 2023 10:48

Glutexo requested changes Jan 16, 2023

View reviewed changes

test/onigumo_cli_test.exs Outdated Show resolved Hide resolved

Glutexo reviewed May 8, 2023

View reviewed changes

test/onigumo_cli_test.exs Outdated Show resolved Hide resolved

nappex added 3 commits May 20, 2023 17:46

Merge branch 'master' into cli

2599c76

Move private function body and prepare_response

8436452

- These functions was moved because we need them in more tests then just one

Rename file from http_util to http

e807364

- when we move the file to folder support util in name is extra. The mean of util is include in folder name support

Glutexo requested changes May 20, 2023

View reviewed changes

mix.exs Outdated Show resolved Hide resolved

nappex added 4 commits May 20, 2023 19:50

Rename module HttpTestUtil to HttpSupport

16ead53

Move prepare input func to support

b083ba7

Test CLI with input for 'Downloader'

71262f7

Format the code with mix

fc5380f

nappex marked this pull request as ready for review May 23, 2023 09:54

nappex mentioned this pull request Jul 29, 2023

Test Onigumo CLI #158

Closed

Glutexo previously approved these changes Jul 29, 2023

View reviewed changes

nappex dismissed Glutexo’s stale review via 435659a July 29, 2023 18:30

Remove prepare in the name of support functions

6b380c7

nappex force-pushed the cli branch from 435659a to 6b380c7 Compare July 30, 2023 12:40

Load default elixirc_paths, then add extra paths to default

0a1a093

nappex force-pushed the cli branch from 118845d to 0a1a093 Compare October 1, 2023 18:10

Fix formatting

39583f7

Glutexo reviewed Oct 1, 2023

View reviewed changes

Reduce calling Mix.env() to just once

63d2f12

Glutexo approved these changes Oct 1, 2023

View reviewed changes

Glutexo merged commit a8425aa into Glutexo:master Oct 1, 2023

Glutexo deleted the cli branch October 1, 2023 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test to invoke onigumo from CLI #159

Add test to invoke onigumo from CLI #159

nappex commented Jan 15, 2023

Glutexo commented Jan 16, 2023 •

edited

Loading

Glutexo commented Jan 16, 2023 •

edited

Loading

Glutexo left a comment

Glutexo commented Feb 6, 2023

Glutexo left a comment

nappex commented May 23, 2023 •

edited

Loading

Glutexo left a comment

Glutexo May 22, 2023

Glutexo Jul 29, 2023 •

edited

Loading

nappex Jul 30, 2023

Glutexo Jul 29, 2023

Glutexo Oct 1, 2023

		defp elixirc_paths(:test), do: ["lib", "test/support"]
		defp elixirc_paths(_), do: ["lib"]

Add test to invoke onigumo from CLI #159

Add test to invoke onigumo from CLI #159

Conversation

nappex commented Jan 15, 2023

Glutexo commented Jan 16, 2023 • edited Loading

Glutexo commented Jan 16, 2023 • edited Loading

Glutexo left a comment

Choose a reason for hiding this comment

Glutexo commented Feb 6, 2023

Glutexo left a comment

Choose a reason for hiding this comment

nappex commented May 23, 2023 • edited Loading

Glutexo left a comment

Choose a reason for hiding this comment

Glutexo May 22, 2023

Choose a reason for hiding this comment

Glutexo Jul 29, 2023 • edited Loading

Choose a reason for hiding this comment

nappex Jul 30, 2023

Choose a reason for hiding this comment

Glutexo Jul 29, 2023

Choose a reason for hiding this comment

Glutexo Oct 1, 2023

Choose a reason for hiding this comment

Glutexo commented Jan 16, 2023 •

edited

Loading

Glutexo commented Jan 16, 2023 •

edited

Loading

nappex commented May 23, 2023 •

edited

Loading

Glutexo Jul 29, 2023 •

edited

Loading