cmds/core/wget: remove testutil by binjip978 · Pull Request #2540 · u-root/u-root

binjip978 · 2022-10-27T06:13:17Z

Signed-off-by: Siarhiej Siemianczuk pdp.eleven11@gmail.com

codecov · 2022-10-27T06:42:48Z

Codecov Report

Base: 73.38% // Head: 73.43% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (5bdbadf) compared to base (24c60d4).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2540      +/-   ##
==========================================
+ Coverage   73.38%   73.43%   +0.05%     
==========================================
  Files         411      411              
  Lines       41746    41751       +5     
==========================================
+ Hits        30635    30661      +26     
+ Misses      11111    11090      -21

Impacted Files	Coverage Δ
pkg/curl/schemes.go	`59.50% <0.00%> (ø)`
cmds/core/wget/wget.go	`68.42% <69.23%> (+68.42%)`	⬆️
cmds/core/date/date.go	`90.83% <0.00%> (-0.77%)`	⬇️
cmds/core/ip/ops_linux.go	`79.82% <0.00%> (+0.87%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rminnich

Just two questions, nice to see more tests!

rminnich

I like this new design. You can see my one comment re New() vs new()
Let's see what abrender thinks but I believe we are almost done!

abrender

Thanks for continuing to work on this - it is appreciated!

binjip978 · 2022-11-10T18:40:49Z

looks like we can't guarantee EOF wget_test.go:143: expected: EOF, got: failed to download http://localhost:33527/200: encountered error Get "http://localhost:33527/200": http: server closed

pervious test just check for non zero, but this seems flaky on CI.

binjip978 · 2022-11-13T12:07:57Z

looks like we can't guarantee EOF wget_test.go:143: expected: EOF, got: failed to download http://localhost:33527/200: encountered error Get "http://localhost:33527/200": http: server closed

pervious test just check for non zero, but this seems flaky on CI.

Can't made it work. It may return at least 3 different errors and one of the is even internal errServerClosedIdle = errors.New("http: server closed idle connection"). So I diced to remove this test, seems like it's not testing anything in wget command, and probably does not do anything pkg/curl either. Error is returned in httpFetch functions in scheme.go in line 393 resp, err := c.Do(req).

Or I can remove errors.Is and check for error code 1 or 0.

abrender · 2022-11-15T00:46:56Z

looks like we can't guarantee EOF wget_test.go:143: expected: EOF, got: failed to download http://localhost:33527/200: encountered error Get "http://localhost:33527/200": http: server closed
pervious test just check for non zero, but this seems flaky on CI.

Can't made it work. It may return at least 3 different errors and one of the is even internal errServerClosedIdle = errors.New("http: server closed idle connection"). So I diced to remove this test, seems like it's not testing anything in wget command, and probably does not do anything pkg/curl either. Error is returned in httpFetch functions in scheme.go in line 393 resp, err := c.Do(req).

Or I can remove errors.Is and check for error code 1 or 0.

Perhaps cmpopts.EquateErrors and AnyError can help here?

import(
    "github.com/google/go-cmp/cmp"
    "github.com/google/go-cmp/cmp/cmpopts"
)

// Update test case to have `err: cmpopts.AnyError` instead of io.EOF

if !cmp.Equal(tt.err, err, cmpopts.EquateErrors()) {
  t.Errorf("Got error=%v, expected=%v", err, tt.err)
}
if (err != nil || tt.err != nil ) {
   return
}
...

binjip978 · 2022-11-15T10:14:42Z

looks like we can't guarantee EOF wget_test.go:143: expected: EOF, got: failed to download http://localhost:33527/200: encountered error Get "http://localhost:33527/200": http: server closed
pervious test just check for non zero, but this seems flaky on CI.

Can't made it work. It may return at least 3 different errors and one of the is even internal errServerClosedIdle = errors.New("http: server closed idle connection"). So I diced to remove this test, seems like it's not testing anything in wget command, and probably does not do anything pkg/curl either. Error is returned in httpFetch functions in scheme.go in line 393 resp, err := c.Do(req).
Or I can remove errors.Is and check for error code 1 or 0.

Perhaps cmpopts.EquateErrors and AnyError can help here?
import(
    "github.com/google/go-cmp/cmp"
    "github.com/google/go-cmp/cmp/cmpopts"
)

// Update test case to have `err: cmpopts.AnyError` instead of io.EOF

if !cmp.Equal(tt.err, err, cmpopts.EquateErrors()) {
  t.Errorf("Got error=%v, expected=%v", err, tt.err)
}
if (err != nil || tt.err != nil ) {
   return
}
...

This will work, but I would prefer to just separate this test case and do simple if err != nil { t.Error() } and don't bring any additional dependencies. This is better aligned with go values "a little copying is better than a little dependency".

The problem with this that I don't fully understand what the purpose of this test case. We already covered error case for curl command with 4xx and 5xx test cases. What kind of http related error we got there it's not a wget command problem, it's maybe a problem of curl lib, but error comes from http client do function.
But I probably missing something.

abrender · 2022-11-15T14:34:56Z

looks like we can't guarantee EOF wget_test.go:143: expected: EOF, got: failed to download http://localhost:33527/200: encountered error Get "http://localhost:33527/200": http: server closed
pervious test just check for non zero, but this seems flaky on CI.

Can't made it work. It may return at least 3 different errors and one of the is even internal errServerClosedIdle = errors.New("http: server closed idle connection"). So I diced to remove this test, seems like it's not testing anything in wget command, and probably does not do anything pkg/curl either. Error is returned in httpFetch functions in scheme.go in line 393 resp, err := c.Do(req).
Or I can remove errors.Is and check for error code 1 or 0.

Perhaps cmpopts.EquateErrors and AnyError can help here?
import(
    "github.com/google/go-cmp/cmp"
    "github.com/google/go-cmp/cmp/cmpopts"
)

// Update test case to have `err: cmpopts.AnyError` instead of io.EOF

if !cmp.Equal(tt.err, err, cmpopts.EquateErrors()) {
  t.Errorf("Got error=%v, expected=%v", err, tt.err)
}
if (err != nil || tt.err != nil ) {
   return
}
...
This will work, but I would prefer to just separate this test case and do simple if err != nil { t.Error() } and don't bring any additional dependencies. This is better aligned with go values "a little copying is better than a little dependency".

The problem with this that I don't fully understand what the purpose of this test case. We already covered error case for curl command with 4xx and 5xx test cases. What kind of http related error we got there it's not a wget command problem, it's maybe a problem of curl lib, but error comes from http client do function. But I probably missing something.

Sure, a separate test for err != nil sounds fine to me :) I was just throwing out a solution if you wanted to keep it in a single test.

As for what it's testing - I'm not the original author, but I assume there's a reasonable assumption that error handling for HTTP status codes is different than error handling for errors unrelated to HTTP status codes (like network errors).

add none nil error to HTTPClientCodeError if error code is not 200 to make errors.Is work Signed-off-by: Siarhiej Siemianczuk <pdp.eleven11@gmail.com>

Signed-off-by: Siarhiej Siemianczuk <pdp.eleven11@gmail.com>

under -race wget can return three different errors and this made errors.Is problematic to use Signed-off-by: Siarhiej Siemianczuk <pdp.eleven11@gmail.com>

Signed-off-by: Siarhiej Siemianczuk <pdp.eleven11@gmail.com>

abrender · 2022-11-21T20:32:08Z

Looks good to me! Thank you so much for your patience.

I'll await @rminnich to resolve his open comment threads

binjip978 · 2022-11-22T13:54:47Z

Looks good to me! Thank you so much for your patience.

I'll await @rminnich to resolve his open comment threads

Thanks for feedback, appreciate it :) Learned a bit about current Fatal behaviour.

binjip978 · 2022-12-18T19:18:24Z

Sorry @rminnich but I can't change labels, so pinging you via mention :)

rminnich

This is a substantial improvement. Thanks for your patience.

rminnich reviewed Oct 27, 2022

View reviewed changes

Comment thread cmds/core/wget/wget.go Outdated

Comment thread cmds/core/wget/wget.go Outdated

binjip978 force-pushed the wget-testuti branch from 7507450 to 56be810 Compare October 28, 2022 04:38

rminnich reviewed Oct 28, 2022

View reviewed changes

Comment thread cmds/core/wget/wget.go Outdated

rminnich added the Awaiting author Waiting for new changes or feedback for author. label Oct 28, 2022

binjip978 force-pushed the wget-testuti branch from 56be810 to 5df6aed Compare October 28, 2022 18:14

rminnich reviewed Oct 30, 2022

View reviewed changes

Comment thread cmds/core/wget/wget.go Outdated

abrender reviewed Oct 31, 2022

View reviewed changes

Comment thread cmds/core/wget/wget.go Outdated

Comment thread cmds/core/wget/wget_test.go Outdated

Comment thread cmds/core/wget/wget_test.go Outdated

binjip978 force-pushed the wget-testuti branch 2 times, most recently from 496f483 to ec09887 Compare November 4, 2022 05:12

binjip978 requested a review from rminnich November 6, 2022 15:10

rminnich reviewed Nov 6, 2022

View reviewed changes

Comment thread cmds/core/wget/wget.go Outdated

abrender reviewed Nov 8, 2022

View reviewed changes

binjip978 force-pushed the wget-testuti branch 2 times, most recently from 95b8156 to 49bd7cb Compare November 9, 2022 12:19

binjip978 requested review from abrender and rminnich and removed request for rminnich November 9, 2022 13:14

abrender reviewed Nov 9, 2022

View reviewed changes

binjip978 force-pushed the wget-testuti branch from 49bd7cb to 8269aa6 Compare November 10, 2022 18:13

abrender reviewed Nov 10, 2022

View reviewed changes

Comment thread cmds/core/wget/wget_test.go Outdated

Comment thread cmds/core/wget/wget_test.go Outdated

binjip978 force-pushed the wget-testuti branch from 8269aa6 to ae3400c Compare November 13, 2022 11:54

binjip978 added 3 commits November 15, 2022 21:24

cmds/core/wget: remove testutil

a48c821

add none nil error to HTTPClientCodeError if error code is not 200 to make errors.Is work Signed-off-by: Siarhiej Siemianczuk <pdp.eleven11@gmail.com>

cmds/core/wget: add command struct for better flag testing

43228e5

Signed-off-by: Siarhiej Siemianczuk <pdp.eleven11@gmail.com>

cmds/core/wget: use httptest for wget server

6305332

Signed-off-by: Siarhiej Siemianczuk <pdp.eleven11@gmail.com>

cmds/core/wget: move out connection close test

c139102

under -race wget can return three different errors and this made errors.Is problematic to use Signed-off-by: Siarhiej Siemianczuk <pdp.eleven11@gmail.com>

binjip978 force-pushed the wget-testuti branch from ae3400c to c139102 Compare November 15, 2022 19:25

cmds/core/wget: test default path using chdir

5235f7e

Signed-off-by: Siarhiej Siemianczuk <pdp.eleven11@gmail.com>

abrender reviewed Nov 18, 2022

View reviewed changes

Comment thread cmds/core/wget/wget_test.go Outdated

Comment thread cmds/core/wget/wget_test.go Outdated

cmds/core/wget: remove filepath and add test cases

cbac26d

Signed-off-by: Siarhiej Siemianczuk <pdp.eleven11@gmail.com>

binjip978 marked this pull request as draft November 22, 2022 06:39

binjip978 marked this pull request as ready for review November 22, 2022 06:39

binjip978 requested a review from rminnich December 18, 2022 19:17

rminnich approved these changes Dec 19, 2022

View reviewed changes

Comment thread cmds/core/wget/wget.go

Merge branch 'main' into wget-testuti

5bdbadf

rminnich approved these changes Dec 19, 2022

View reviewed changes

rminnich merged commit bac9c58 into u-root:main Dec 19, 2022

binjip978 deleted the wget-testuti branch December 19, 2022 08:24

Conversation

binjip978 commented Oct 27, 2022

Uh oh!

codecov Bot commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rminnich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rminnich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abrender left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

binjip978 commented Nov 10, 2022

Uh oh!

Uh oh!

Uh oh!

binjip978 commented Nov 13, 2022

Uh oh!

abrender commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

binjip978 commented Nov 15, 2022

Uh oh!

abrender commented Nov 15, 2022

Uh oh!

Uh oh!

Uh oh!

abrender commented Nov 21, 2022

Uh oh!

binjip978 commented Nov 22, 2022

Uh oh!

binjip978 commented Dec 18, 2022

Uh oh!

rminnich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Oct 27, 2022 •

edited

Loading

abrender commented Nov 15, 2022 •

edited

Loading