Increase coverage #542

szmarczak · 2018-08-02T15:41:12Z

No description provided.

szmarczak · 2018-08-02T16:16:51Z

I'll leave this part:

Lines 30 to 32 in 6ba9e68

    
           if (request[reentry]) { 
        
           	return; 
        
           }

this makes us sure timed-out isn't called twice or more, although it's obvious we don't do it, so can't make a test for that.

szmarczak · 2018-08-02T16:49:23Z

We also need tests for electron support.

szmarczak · 2018-08-02T17:14:02Z

I just wonder if this piece of code is needed:

got/source/get-body-size.js

Lines 31 to 33 in 6ba9e68

    
           if (is.nodeStream(body) && is.buffer(body._buffer)) { 
        
           	return body._buffer.length; 
        
           }

because I tried passing a Buffer as body and this doesn't even get called.

szmarczak · 2018-08-02T17:24:09Z

Note: Missing test for #490 and for:

got/source/progress.js

Lines 40 to 43 in 6ba9e68

    
           // Prevent the known issue of `bytesWritten` being larger than body size 
        
           if (uploadBodySize && uploaded > uploadBodySize) { 
        
           	uploaded = uploadBodySize; 
        
           }

szmarczak · 2018-08-02T17:27:48Z

source/normalize-arguments.js

-				if (is.number(after)) {
-					after *= 1000;
+				if (is.nan(after)) {
+					after = Date.parse(error.headers['retry-after']) - Date.now();


This fixes a bug: at first I thought is.number won't return true for NaN, but I was wrong.

Math.max is not needed BTW.

sindresorhus · 2018-08-02T17:59:50Z

source/request-as-event-emitter.js

-				} catch (error2) {
-					emitter.emit('error', error2);
-				}
-			}, backoff, options);


Why was this removed?

get() does not throw any errors directly. It uses emitter.emit('error', error);

Good point. I think we need a try/catch here though: https://github.com/sindresorhus/got/pull/542/files#diff-62bdc57f6f22ae58f495daef16f21f8bR141 In case the options.gotRetry.retries function throws.

I see I've added it here. It's no longer needed because decodeURI error is catched here. If it wasn't there then we would need to catch on setTimeout too. But there's no need as metioned :)

I'll deny my answer:

It's caught here, but not here and not here.

It was caught nowhere. I don't know how I did get that 😕

To sum up: the removal is good, I'm sure 10000% :)

I hope my answer is no confusing :P

Your answer is not confusing, but you're not answering my question. To be clear, I think we need to try/catch this line because it could throw: const backoff = options.gotRetry.retries(++retryTries, error);

you're not answering my question.

Sorry, if you're talking about the second one then it hadn't popped up when I was writing the answer. Pardon my confusion.

I think we need to try/catch this line because it could throw

Good point! Indeed. 🙌

I think we need to try/catch this line because it could throw
Good point! Indeed. 🙌

And of course a test ;)

Done! I hope it satisfies you :)

sindresorhus · 2018-08-02T18:01:07Z

because I tried passing a Buffer as body and this doesn't even get called.

You have to pass a stream. The buffer testing is testing a property on the stream.

szmarczak · 2018-08-02T18:44:39Z

@sindresorhus But it's converted to a stream here:

got/source/normalize-arguments.js

Lines 111 to 114 in 6ba9e68

    
           if (is.buffer(body)) { 
        
           	options.body = toReadableStream(body); 
        
           	options.body._buffer = body; 
        
           }

sindresorhus · 2018-08-02T19:14:17Z

@szmarczak Hmm, then maybe it returns here:

got/source/get-body-size.js

Line 10 in 6ba9e68

if (options.headers['content-length']) {

Add a debugger statement to each if and figure out where it returns.

szmarczak · 2018-08-02T19:28:45Z

Hmm, then maybe it returns here:

Yeah, you're right, checked it. So, is it needed? IMO we can get rid of that.

sindresorhus · 2018-08-02T19:45:32Z

Yeah, you're right, checked it. So, is it needed? IMO we can get rid of that.

See:

got/source/request-as-event-emitter.js

Lines 163 to 166 in 4d92eb6

    
           // This is the second try at setting a `content-length` header. 
        
           // This supports getting the size async, in contrast to 
        
           // https://github.com/sindresorhus/got/blob/82763c8089596dcee5eaa7f57f5dbf8194842fe6/index.js#L579-L582 
        
           // TODO: We should unify these two at some point

This should be done in a separate PR though

sindresorhus · 2018-08-02T19:48:07Z

test/retry.js

+				throw new Error('Simple error');
+			}
+		}
+	}), 'Simple error');


It's a good practice to put the message in a variable and use it in both to make absolutely sure they are the same and stay the same in the future too.

szmarczak · 2018-08-02T20:03:06Z

This should be done in a separate PR though

So it will be :)

It's a good practice to put the message in a variable and use it in both to make absolutely sure they are the same and stay the same in the future too.

Done 🦄

sindresorhus · 2018-08-02T20:10:59Z

We also need tests for electron support.

Nah, don't care. If it prevents us from getting 100% test coverage, we can just mock it.

Note: Missing test for #490 and for:

Do you intend to do that in this PR? Asking so I know whether this is ready to merge or not.

sindresorhus · 2018-08-02T20:11:31Z

Something is failing here: https://ci.appveyor.com/project/sindresorhus/got/build/1.0.245/job/nunr63w0rttwjv0t

szmarczak · 2018-08-02T20:16:12Z

If it prevents us from getting 100% test coverage

The point is: not only that keeps us from 100%. But it's possible to achieve 99% :)

Do you intend to do that in this PR? Asking so I know whether this is ready to merge or not.

No, that'll be a separate PR too. Yes, it's ready to merge now :)

szmarczak · 2018-08-02T20:21:34Z

Something is failing here

AppVeyor is damn slow. Increasing the timeouts should fix the problem I think. If it does not, I'll fix it in the morning as it's 10:21 PM here.

szmarczak · 2018-08-02T20:29:06Z

See? Fixed :)

sindresorhus · 2018-08-03T04:27:22Z

Yup, AppVeyor is annoying. We had so many problems with it over at https://github.com/avajs/ava too...

sindresorhus · 2018-08-03T04:28:21Z

👍 I'm extra happy that doing this actually caught a couple of potential bugs.

szmarczak and others added 4 commits August 2, 2018 17:40

Increase normalize-arguments.js coverage

c415146

Update normalize-arguments.js

7148bef

Update normalize-arguments.js

94fac09

Increase errors.js coverage

c77c011

szmarczak and others added 3 commits August 2, 2018 18:30

Increase coverage for as-stream.js

ec09eef

Update request-as-event-emitter.js

ed7489e

Remove not needed try/catch

a86e47e

szmarczak changed the title ~~[WIP] 100% coverage~~ Increase coverage Aug 2, 2018

szmarczak commented Aug 2, 2018

View reviewed changes

sindresorhus reviewed Aug 2, 2018

View reviewed changes

Catch the retry function

93f4408

sindresorhus reviewed Aug 2, 2018

View reviewed changes

Update retry.js

59b6f16

Update timeout.js

3609221

sindresorhus merged commit 10d22b7 into sindresorhus:master Aug 3, 2018

szmarczak added a commit to szmarczak/got that referenced this pull request Aug 3, 2018

Increase coverage (sindresorhus#542)

8a7a2c1

szmarczak deleted the 100-coverage branch January 17, 2019 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase coverage #542

Increase coverage #542

szmarczak commented Aug 2, 2018

szmarczak commented Aug 2, 2018 •

edited

Loading

szmarczak commented Aug 2, 2018

szmarczak commented Aug 2, 2018

szmarczak commented Aug 2, 2018 •

edited

Loading

szmarczak Aug 2, 2018

sindresorhus Aug 2, 2018

szmarczak Aug 2, 2018

sindresorhus Aug 2, 2018

szmarczak Aug 2, 2018

szmarczak Aug 2, 2018

sindresorhus Aug 2, 2018 •

edited

Loading

szmarczak Aug 2, 2018

sindresorhus Aug 2, 2018

szmarczak Aug 2, 2018

sindresorhus commented Aug 2, 2018

szmarczak commented Aug 2, 2018

sindresorhus commented Aug 2, 2018 •

edited

Loading

szmarczak commented Aug 2, 2018

sindresorhus commented Aug 2, 2018 •

edited

Loading

sindresorhus Aug 2, 2018

szmarczak Aug 2, 2018

szmarczak commented Aug 2, 2018

sindresorhus commented Aug 2, 2018

sindresorhus commented Aug 2, 2018

szmarczak commented Aug 2, 2018

szmarczak commented Aug 2, 2018

szmarczak commented Aug 2, 2018

sindresorhus commented Aug 3, 2018

sindresorhus commented Aug 3, 2018

Increase coverage #542

Increase coverage #542

Conversation

szmarczak commented Aug 2, 2018

szmarczak commented Aug 2, 2018 • edited Loading

szmarczak commented Aug 2, 2018

szmarczak commented Aug 2, 2018

szmarczak commented Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Aug 2, 2018

szmarczak commented Aug 2, 2018

sindresorhus commented Aug 2, 2018 • edited Loading

szmarczak commented Aug 2, 2018

sindresorhus commented Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szmarczak commented Aug 2, 2018

sindresorhus commented Aug 2, 2018

sindresorhus commented Aug 2, 2018

szmarczak commented Aug 2, 2018

szmarczak commented Aug 2, 2018

szmarczak commented Aug 2, 2018

sindresorhus commented Aug 3, 2018

sindresorhus commented Aug 3, 2018

szmarczak commented Aug 2, 2018 •

edited

Loading

szmarczak commented Aug 2, 2018 •

edited

Loading

sindresorhus Aug 2, 2018 •

edited

Loading

sindresorhus commented Aug 2, 2018 •

edited

Loading

sindresorhus commented Aug 2, 2018 •

edited

Loading