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

Fix LoadHTML* tests #1559

Merged
merged 2 commits into from
Oct 16, 2018
Merged

Conversation

sponomarev
Copy link
Contributor

@sponomarev sponomarev commented Sep 19, 2018

Digging into the test code base I've found out that some of the tests for LoadHTML* methods are not reliable and efficient. They use timeouts to be sure that goroutine with the server has started. And even more, in old implementation, the server started only once – all the new instances silently failed due to the occupied network port.

Here is a short overview of the proposed changes:

  • it's not necessary to rely on timeouts, the server starts listening synchronously and returns control when it is ready
  • once the server is run, it's stopped after a test passes
  • dry out http server setup
  • magic with empty closure return is eliminated
  • preserve router.RunTLS coverage with integration tests

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #1559 into master will decrease coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
- Coverage   99.06%   98.79%   -0.27%     
==========================================
  Files          39       39              
  Lines        1915     1915              
==========================================
- Hits         1897     1892       -5     
- Misses         14       19       +5     
  Partials        4        4
Impacted Files Coverage Δ
gin.go 95.63% <0%> (-2.43%) ⬇️

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 b27b702...1b7b5fa. Read the comment docs.

3 similar comments
@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #1559 into master will decrease coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
- Coverage   99.06%   98.79%   -0.27%     
==========================================
  Files          39       39              
  Lines        1915     1915              
==========================================
- Hits         1897     1892       -5     
- Misses         14       19       +5     
  Partials        4        4
Impacted Files Coverage Δ
gin.go 95.63% <0%> (-2.43%) ⬇️

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 b27b702...1b7b5fa. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #1559 into master will decrease coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
- Coverage   99.06%   98.79%   -0.27%     
==========================================
  Files          39       39              
  Lines        1915     1915              
==========================================
- Hits         1897     1892       -5     
- Misses         14       19       +5     
  Partials        4        4
Impacted Files Coverage Δ
gin.go 95.63% <0%> (-2.43%) ⬇️

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 b27b702...1b7b5fa. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #1559 into master will decrease coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
- Coverage   99.06%   98.79%   -0.27%     
==========================================
  Files          39       39              
  Lines        1915     1915              
==========================================
- Hits         1897     1892       -5     
- Misses         14       19       +5     
  Partials        4        4
Impacted Files Coverage Δ
gin.go 95.63% <0%> (-2.43%) ⬇️

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 b27b702...1b7b5fa. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #1559 into master will decrease coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
- Coverage   99.27%   99.05%   -0.22%     
==========================================
  Files          39       39              
  Lines        1928     1913      -15     
==========================================
- Hits         1914     1895      -19     
- Misses         10       14       +4     
  Partials        4        4
Impacted Files Coverage Δ
gin.go 98.05% <0%> (-1.95%) ⬇️
debug.go 95% <0%> (-0.75%) ⬇️
recovery.go 100% <0%> (ø) ⬆️
tree.go 100% <0%> (ø) ⬆️

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 524757b...a2a232d. Read the comment docs.

@sponomarev sponomarev force-pushed the fix/load-html-tests branch 2 times, most recently from d193cb3 to 2609878 Compare September 20, 2018 18:07
- it's not necessary to rely on timeout, server starts listening synchronously and returns control when it is ready
- once the server is run, it's stopped after test passes
- dry out http server setup
- preserve router.RunTLS coverage with integration tests
@thinkerou
Copy link
Member

@sponomarev thanks a lot, LGTM, and need @appleboy review.

@appleboy appleboy added this to the 1.4 milestone Oct 16, 2018
@appleboy appleboy merged commit cfa092f into gin-gonic:master Oct 16, 2018
justinfx pushed a commit to justinfx/gin that referenced this pull request Nov 3, 2018
Digging into the test code base I've found out that some of the tests for `LoadHTML*` methods are not reliable and efficient. They use timeouts to be sure that goroutine with the server has started. And even more, in old implementation, the server started only once – all the new instances silently failed due to the occupied network port.

Here is a short overview of the proposed changes: 
- it's not necessary to rely on timeouts, the server starts listening synchronously and returns control when it is ready
- once the server is run, it's stopped after a test passes
- dry out http server setup
- magic with empty closure return is eliminated 
- preserve router.RunTLS coverage with integration tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants