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

pool: fix loop variables in tests #14

Merged
merged 1 commit into from
Jan 5, 2023
Merged

Conversation

bobheadxi
Copy link
Member

@bobheadxi bobheadxi commented Jan 5, 2023

This has been a long-standing discussion (golang/go#20733, golang/go#56010) but today, variables in iterations are per-loop, not per-iteration, so you need to copy these values for them to be used inside p.Go.

Add a t.Log inside p.Go in the corresponding tests to see that previously, we're always comparing against the last value in the iteration, i.e. 100, and after the change the correct value is used:

before - always the last value after - uses the correct value
=== CONT  TestContextPool/limit/100
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/10
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/100
    context_pool_test.go:128: 100
    context_pool_test.go:128: 100
    context_pool_test.go:128: 100
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/10
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/1
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/100
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/10
    context_pool_test.go:128: 100
    context_pool_test.go:128: 100
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/1
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/10
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/1
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/10
    context_pool_test.go:128: 100
    context_pool_test.go:128: 100
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/1
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/10
    context_pool_test.go:128: 100
    context_pool_test.go:128: 100
=== CONT  TestContextPool/limit/1
    context_pool_test.go:130: 1
=== CONT  TestContextPool/limit/100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
=== CONT  TestContextPool/limit/10
    context_pool_test.go:130: 10
=== CONT  TestContextPool/limit/100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100
    context_pool_test.go:130: 100

@bobheadxi bobheadxi requested a review from camdencheek January 5, 2023 17:00
@codecov-commenter
Copy link

Codecov Report

Merging #14 (77ca0f9) into main (73ebfbf) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #14   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files          10       10           
  Lines         350      350           
=======================================
  Hits          348      348           
  Misses          2        2           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@camdencheek
Copy link
Member

Good catch, thanks!

@camdencheek camdencheek merged commit ef4bcf7 into main Jan 5, 2023
@camdencheek camdencheek deleted the pool/fix-benchmark-loop-vars branch January 5, 2023 17:09
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.

3 participants