Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/k6 773 #905

Merged
merged 1 commit into from Aug 28, 2019
Merged

Bugfix/k6 773 #905

merged 1 commit into from Aug 28, 2019

Conversation

openmohan
Copy link
Contributor

No description provided.

Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better. Thanks again for the PR. Still some things to fix.
You could've also used the old PR and just pushed there but this is fine as well :)
You can also rebase on top of master and force push to your branch in order to fix the golint check

js/modules/k6/http/response.go Outdated Show resolved Hide resolved
js/modules/k6/http/response.go Outdated Show resolved Hide resolved
js/modules/k6/http/response.go Outdated Show resolved Hide resolved
js/modules/k6/http/response_test.go Outdated Show resolved Hide resolved
js/modules/k6/http/response.go Outdated Show resolved Hide resolved
@openmohan
Copy link
Contributor Author

Thanks for the review @mstoykov . I will work on them

@openmohan openmohan changed the title Bugfix/k6 773 [WIP]Bugfix/k6 773 Feb 27, 2019
@openmohan openmohan force-pushed the bugfix/k6-773 branch 4 times, most recently from e8ef74f to ee38c56 Compare March 23, 2019 04:48
@codecov
Copy link

codecov bot commented Mar 23, 2019

Codecov Report

Merging #905 into master will decrease coverage by 0.04%.
The diff coverage is 64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #905      +/-   ##
==========================================
- Coverage   72.08%   72.04%   -0.05%     
==========================================
  Files         131      131              
  Lines        9602     9626      +24     
==========================================
+ Hits         6922     6935      +13     
- Misses       2267     2275       +8     
- Partials      413      416       +3
Impacted Files Coverage Δ
js/modules/k6/http/response.go 77.06% <ø> (ø) ⬆️
lib/netext/httpext/response.go 65.07% <64%> (-6.72%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 651146b...3adc4a2. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 23, 2019

Codecov Report

Merging #905 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #905      +/-   ##
==========================================
+ Coverage   72.31%   72.34%   +0.03%     
==========================================
  Files         132      132              
  Lines        9703     9721      +18     
==========================================
+ Hits         7017     7033      +16     
- Misses       2272     2273       +1     
- Partials      414      415       +1
Impacted Files Coverage Δ
lib/netext/httpext/response.go 80.7% <100%> (+8.9%) ⬆️
core/engine.go 92.99% <0%> (-0.94%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fc300c...6ae3697. Read the comment docs.

js/modules/k6/http/response_test.go Outdated Show resolved Hide resolved
js/modules/k6/http/response_test.go Outdated Show resolved Hide resolved
@openmohan openmohan force-pushed the bugfix/k6-773 branch 2 times, most recently from 1b8e8ab to edac6cf Compare March 23, 2019 07:01
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
@openmohan openmohan force-pushed the bugfix/k6-773 branch 4 times, most recently from 29ab622 to a63b509 Compare March 23, 2019 09:36
@openmohan openmohan changed the title [WIP]Bugfix/k6 773 Bugfix/k6 773 Mar 23, 2019
.golangci.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR, it is getting better, but we(I) did some changes to the js/module/k6/http and now some of the code should be in the httpext package instead. As well as we added golangci-lint and we would like to get lines that are under 120 character where possible

@openmohan
Copy link
Contributor Author

Thanks again for the PR, it is getting better, but we(I) did some changes to the js/module/k6/http and now some of the code should be in the httpext package instead. As well as we added golangci-lint and we would like to get lines that are under 120 character where possible

I will work on it this weekend and make it done. Thanks for the review

lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
lib/netext/httpext/response_test.go Outdated Show resolved Hide resolved
@openmohan openmohan force-pushed the bugfix/k6-773 branch 9 times, most recently from 8b295af to 2866699 Compare April 14, 2019 12:04
@openmohan
Copy link
Contributor Author

@na-- @mstoykov Not sure if the test-prev-golang test cases are failing due to my change. Please let me know if I am missing something. Thanks

Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again.
It is almost perfect, now!

lib/netext/httpext/response.go Show resolved Hide resolved
lib/netext/httpext/response.go Show resolved Hide resolved
@openmohan
Copy link
Contributor Author

@mstoykov Thanks for the review.

I have made the changes, please let me know If I missed any.

@CLAassistant
Copy link

CLAassistant commented Aug 4, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay .. again :(

It looks very well to me now 👍

@codecov-io
Copy link

codecov-io commented Aug 8, 2019

Codecov Report

Merging #905 into master will decrease coverage by 0.46%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #905      +/-   ##
==========================================
- Coverage    73.2%   72.74%   -0.47%     
==========================================
  Files         141      133       -8     
  Lines       10249     9861     -388     
==========================================
- Hits         7503     7173     -330     
+ Misses       2303     2273      -30     
+ Partials      443      415      -28
Impacted Files Coverage Δ
lib/netext/httpext/response.go 80.7% <100%> (-7.18%) ⬇️
lib/netext/httpext/transport.go 89.65% <0%> (-6.5%) ⬇️
lib/netext/httpext/request.go 88.33% <0%> (-4.98%) ⬇️
lib/netext/tls.go 46.93% <0%> (-2.09%) ⬇️
cmd/config.go 74.82% <0%> (-1.21%) ⬇️
cmd/run.go 9.21% <0%> (-0.3%) ⬇️
lib/options.go 92.67% <0%> (-0.16%) ⬇️
cmd/root.go 34% <0%> (ø) ⬆️
cmd/options.go 66.37% <0%> (ø) ⬆️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd6d2d9...0ec0067. Read the comment docs.

@mstoykov mstoykov added this to the v0.26.0 milestone Aug 8, 2019
@openmohan
Copy link
Contributor Author

Is there a second review or it can be merged? @mstoykov

Thanks for the review

@mstoykov
Copy link
Collaborator

mstoykov commented Aug 8, 2019

@openmohan , @na-- might review the code as well, but we are currently testing the current master in order to release 0.25.1 and we are going to merge your PR next week, after we release it :)

Can you also rebase on the current master as there are now conflicts because I took too long to review this 😭

@openmohan
Copy link
Contributor Author

Can you also rebase on the current master as there are now conflicts because I took too long to review this 😭

Sure, @mstoykov . No issues 👍

@openmohan openmohan force-pushed the bugfix/k6-773 branch 2 times, most recently from 8f1cb68 to 1a52600 Compare August 9, 2019 06:04
lib/netext/httpext/response.go Outdated Show resolved Hide resolved
lib/netext/httpext/response.go Outdated Show resolved Hide resolved
instead of dumping entire JSON, offset, line number and column numbers are displayed
@mstoykov mstoykov merged commit 458316a into grafana:master Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants