Test download of multiple URLs #29

nappex · 2022-01-16T19:49:05Z

From @Glutexo: This required a slight change in the executive code so that the download function can accept a list of URLs without initializing the real environment application.

- the code can not be tested because of func main and constants (resp. atrributes) - add on func more which can be tested easier than main - add two vars, output file and input file - improve test to multiply URLs to handle new func with two args

lib/onigumo.ex

test/onigumo_test.exs

lib/onigumo.ex

Co-authored-by: Glutexo <driezasson@me.com>

To avoid accidentally hitting a real URL.

Glutexo · 2022-01-21T19:49:12Z

I pushed some more cleanup with a goal being minimize the amount of changes even if that would mean more things to fix up in a follow-up pull requests. Such reverts are:

Argument order for the polymorphic download method. Having url(s) as a first argument would be more practical, but currently it is on the second position. It also makes the new Enum.map/2 call identical to the original one. 1ed097e Addressed in Swap download/2 argument order #34.
Test constants names. @filename and @testfile_with_urls are ugly and inconsistent, but do not impose any bug. 5b072f8 Addressed in Unify attribute names #35.
Newlines at the end of mock body strings. They should be there, but aren’t necessary for the tests to pass. 7917810 Addressed in Add newline to the end of the response body #36.
String composition in the URLs input file from interpolation to concatenation. 2456e13
Unwanted spaces in the URLs input file. 2125115, 2f1bc52 Addressed by Remove unwanted space from URL list #37.

I did some more changes that may be worth more cleaning, but I am still trying to minimize the number of changes. Those are:

Renamed the save_urls_contents/1 method to download/1. It does not clash with the other download methods because of its different signature. To me it feels correct as the URLs loaded from the file are sort of a default value. d05f903
Added a is_bitstring guard to the single URL download/2 for symmetry with the multiple URLs counterpart.
~~Change test URLs from onigumo.org to onigumo.local to prevent hitting a real-world servers in case we’d break our mocking.~~ ffc13a7 Extracted to Change test URL domain to local #48.
Improved the test names a bit. 5b5684b
Replaced the @url_1 and @url_2 constants with a single list @urls. That adds some complexity because Enum.at/2 is needed to access the elements. It however introduces nice semantics with Enum.at(@urls, -1) to grab the last URL from the list. This will need some cleanup though. 4373a3b
Removed non-descriptive comments with no real added value. ef2dde3
Used the actual test URL count as an argument for mock expect/4. 6263673
Used the actual test URL count to assert HTTP responses. 63604f4
Added missing HTTP response assertion. 63604f4

Please review and squash+merge if 🆗 .

nappex · 2022-01-29T13:09:10Z

check the merge in commit c38833c, please. @Glutexo

nappex

Check this merge, please @Glutexo

Glutexo · 2022-01-29T15:42:51Z

The c38833c commit only resolves the conflicting lines. A full merge should however bring the introduced change, here usage of the new get!/1 function, to the existing code. It can be either done as a part of the merge commit, or in a separate follow-up commit.

Now, only the “download a single URL” test uses the mock get!/1 function. The remaining tests that were introduced in this pull request and thus did not cause a merge conflict however still use an inline lambda function. This function is the same in all the tests.

nappex · 2022-01-29T17:58:38Z

The c38833c commit only resolves the conflicting lines. A full merge should however bring the introduced change, here usage of the new get!/1 function, to the existing code. It can be either done as a part of the merge commit, or in a separate follow-up commit.

Now, only the “download a single URL” test uses the mock get!/1 function. The remaining tests that were introduced in this pull request and thus did not cause a merge conflict however still use an inline lambda function. This function is the same in all the tests.
* [x]  [“download a single URL”](https://github.com/Glutexo/onigumo/commit/c38833c4531c92bd2d50555c478150944c0d99ee#diff-f6d51c7b96560cf0270cb7e8e078d59afb32389bebe1079e22b586a9e3480baeR16)

* [x]  [“download multiple URLs”](https://github.com/Glutexo/onigumo/commit/c38833c4531c92bd2d50555c478150944c0d99ee#diff-f6d51c7b96560cf0270cb7e8e078d59afb32389bebe1079e22b586a9e3480baeR26)

* [x]  [“download URLs from the input file”](https://github.com/Glutexo/onigumo/commit/c38833c4531c92bd2d50555c478150944c0d99ee#diff-f6d51c7b96560cf0270cb7e8e078d59afb32389bebe1079e22b586a9e3480baeR48)

Yes you are absolutely right, Sorry and thanks for check.
See e12d44c

Glutexo

You nailed it! Requesting just some formatting changes.

test/onigumo_test.exs

nappex · 2022-01-29T18:25:36Z

Merged test semantic 23901c4

Co-authored-by: Glutexo <driezasson@me.com>

Glutexo

Thanks for the fix. The last merge looks good. Reverting to ✅ .

nappex · 2022-01-29T18:31:28Z

Thanks for the fix. The last merge looks good. Reverting to white_check_mark .

grrhh, stupid mistakes caused by my haste. thanks for review.

nappex · 2022-01-29T18:56:10Z

merged added the body func -> e2064af

test/onigumo_test.exs

Glutexo · 2022-01-29T20:29:11Z

Awesome! One step closer.

Glutexo · 2022-01-29T21:34:17Z

The only thing that has been extracted from this pull request and is not merged yet is:

Test load multiple URLs #47

That pull request is the only one I’d insist on merging prior to merging this one.

My ideas for improvement that have not been merged yet are:

Those can be merged first, but they do not necessarily block this pull request. If you’d like to have your pull request merged, I’ll happily update those mentioned after that.

All other currently open pull requests are not related to this one and can be addressed completely indepentently.

Glutexo · 2022-01-30T11:02:58Z

To make the merge history more clear. These merges addressed things that were once already in this pull request, but have been extracted to minimize the number of changes:

Opposed to that, the following merges brought in changes from pull requests containing either my own improvement ideas or things that were only discussed. Those may have sourced from working on this pull request, but never were a part of it.

25ee3c9 Rename http_client to http #38
dedfe8d Consolidate variable names #39 with changes suggested by @nappex
e12d44c Extract mock get! function #41
23901c4 Make asserts more consistent and semantic #42

Providing these lists to distinguish what was a part of this pull request and is this good to address prior to merging this one. Other things don’t need to block this, although it is perfectly possible to merge them first as they are generally small changes.

Glutexo

I have only a suggestion to keep variable lifespan as short as possible. That means, initialize a variable as close to its first (or even only) usage as possible. I don’t insist on fixing it in this pull request. Either fix and merge, or just merge.

test/onigumo_test.exs

Glutexo · 2022-02-03T15:13:17Z

Wow. The whitespace fix in e914eb7 created a fat conflict. Update: Resolved.

add test multiply urls

1fda09b

nappex force-pushed the add_tests branch from dad77c8 to 1fda09b Compare January 16, 2022 20:01

nappex marked this pull request as draft January 16, 2022 20:01

nappex added 5 commits January 19, 2022 13:13

Hack the code to run test for multiply URLs

a7749aa

Allow the expect can be called twice

d0f399d

Use mix format

d87fc80

Remove unused constants

fb88683

nappex marked this pull request as ready for review January 19, 2022 13:12

Glutexo requested changes Jan 20, 2022

View reviewed changes

lib/onigumo.ex Outdated Show resolved Hide resolved

lib/onigumo.ex Outdated Show resolved Hide resolved

test/onigumo_test.exs Outdated Show resolved Hide resolved

lib/onigumo.ex Outdated Show resolved Hide resolved

Fix snake case style, keep lowercase

65e06aa

Co-authored-by: Glutexo <driezasson@me.com>

nappex requested a review from Glutexo January 20, 2022 10:38

Glutexo and others added 14 commits January 21, 2022 19:14

Extract URL download to a polymorphic method

3f4ea7e

Fix wrong name of arg

93f0775

Reveert mode for writing to file

251728a

Add integration test and download multiple URLs

75749f5

Remove forgotten debug

a85a95c

Unparametrize input/output

6b5de23

Keep parametrization of load_urls

2e9cd3d

Rename save_urls_contents to download

d05f903

Change test URLs to .local

ffc13a7

To avoid accidentally hitting a real URL.

Improve test names

5b5684b

Do not change argument URLs

1ed097e

Do not change constant names

5b072f8

Do not change expected body

7917810

Use a URL list instead of two strings

4373a3b

Glutexo assigned nappex Jan 21, 2022

Glutexo assigned nappex and unassigned nappex Jan 21, 2022

nappex added 4 commits January 28, 2022 18:24

Merge branch 'elixir' into add_tests

b4037ce

Merge branch 'elixir' into add_tests

25ee3c9

Merge branch 'elixir' into add_tests

dedfe8d

Merge branch 'elixir' into add_tests

6617f70

nappex commented Jan 29, 2022

View reviewed changes

Glutexo mentioned this pull request Jan 29, 2022

Always use env HTTP client #53

Merged

Merge branch 'elixir' into add_tests

e12d44c

nappex force-pushed the add_tests branch from c38833c to e12d44c Compare January 29, 2022 17:59

Glutexo requested changes Jan 29, 2022

View reviewed changes

test/onigumo_test.exs Outdated Show resolved Hide resolved

test/onigumo_test.exs Outdated Show resolved Hide resolved

Merge branch 'elixir' into add_tests

23901c4

Format the code in expect funcs

a176f15

Co-authored-by: Glutexo <driezasson@me.com>

Glutexo approved these changes Jan 29, 2022

View reviewed changes

Merge branch 'elixir' into add_tests

e2064af

Glutexo reviewed Jan 29, 2022

View reviewed changes

test/onigumo_test.exs Outdated Show resolved Hide resolved

Merge branch 'elixir' into add_tests

57602e9

Glutexo approved these changes Feb 3, 2022

View reviewed changes

test/onigumo_test.exs Outdated Show resolved Hide resolved

test/onigumo_test.exs Show resolved Hide resolved

test/onigumo_test.exs Show resolved Hide resolved

test/onigumo_test.exs Show resolved Hide resolved

test/onigumo_test.exs Show resolved Hide resolved

Merge branch 'elixir' into add_tests

e90bccd

Glutexo merged commit f2df4cb into Glutexo:elixir Feb 5, 2022

Glutexo deleted the add_tests branch February 5, 2022 11:55

Glutexo mentioned this pull request Feb 5, 2022

Consolidate instruction order in tests #57

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test download of multiple URLs #29

Test download of multiple URLs #29

nappex commented Jan 16, 2022 •

edited by Glutexo

Loading

Glutexo commented Jan 21, 2022 •

edited

Loading

nappex commented Jan 29, 2022

nappex left a comment

Glutexo commented Jan 29, 2022 •

edited by nappex

Loading

nappex commented Jan 29, 2022 •

edited

Loading

Glutexo left a comment

nappex commented Jan 29, 2022

Glutexo left a comment •

edited

Loading

nappex commented Jan 29, 2022

nappex commented Jan 29, 2022

Glutexo commented Jan 29, 2022

Glutexo commented Jan 29, 2022 •

edited

Loading

Glutexo commented Jan 30, 2022 •

edited

Loading

Glutexo left a comment

Glutexo commented Feb 3, 2022 •

edited

Loading

Test download of multiple URLs #29

Test download of multiple URLs #29

Conversation

nappex commented Jan 16, 2022 • edited by Glutexo Loading

Glutexo commented Jan 21, 2022 • edited Loading

nappex commented Jan 29, 2022

nappex left a comment

Choose a reason for hiding this comment

Glutexo commented Jan 29, 2022 • edited by nappex Loading

nappex commented Jan 29, 2022 • edited Loading

Glutexo left a comment

Choose a reason for hiding this comment

nappex commented Jan 29, 2022

Glutexo left a comment • edited Loading

Choose a reason for hiding this comment

nappex commented Jan 29, 2022

nappex commented Jan 29, 2022

Glutexo commented Jan 29, 2022

Glutexo commented Jan 29, 2022 • edited Loading

Glutexo commented Jan 30, 2022 • edited Loading

Glutexo left a comment

Choose a reason for hiding this comment

Glutexo commented Feb 3, 2022 • edited Loading

nappex commented Jan 16, 2022 •

edited by Glutexo

Loading

Glutexo commented Jan 21, 2022 •

edited

Loading

Glutexo commented Jan 29, 2022 •

edited by nappex

Loading

nappex commented Jan 29, 2022 •

edited

Loading

Glutexo left a comment •

edited

Loading

Glutexo commented Jan 29, 2022 •

edited

Loading

Glutexo commented Jan 30, 2022 •

edited

Loading

Glutexo commented Feb 3, 2022 •

edited

Loading