- 
                Notifications
    
You must be signed in to change notification settings  - Fork 60
 
tests: replace benchmarks #477
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
base: master
Are you sure you want to change the base?
Conversation
6008328    to
    a95a5cd      
    Compare
  
            
          
                tarantool_test.go
              
                Outdated
          
        
      | b.Errorf("Failed to replace: %s", err) | ||
| } | ||
| rps := float64(requests) / duration.Seconds() | ||
| fmt.Println("requests :", requests) // nolint | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use t.Log/t.Logf instead of fmt.Println here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, thank you.
        
          
                tarantool_test.go
              
                Outdated
          
        
      | } | ||
| } | ||
| }() | ||
| _, err := conns[0].Replace(spaceNo, []interface{}{uint(1111), "hello", "world"}) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not use deprecated APIs in new/modified benches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
| // TestPerformance is a benchmark for the async API that is unable to implement | ||
| // with a Go-benchmark. It can be used to test performance with different | ||
| // numbers of connections and processing goroutines. | ||
| func TestBenchmarkAsync(t *testing.T) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not strongly agains that kind of "bench", but i'd suggest it to be t.Skip()ped by default, only run it in case of CLI argument is provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking: we can configure this bench using CLI flags too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added t.Skip().
| // TestPerformance is a benchmark for the async API that is unable to implement | ||
| // with a Go-benchmark. It can be used to test performance with different | ||
| // numbers of connections and processing goroutines. | ||
| func TestBenchmarkAsync(t *testing.T) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer this test to be separated from usual integration tests (for example - in other file). The same thing applies to usual benches, but again - no prio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer consistency across the project. All integration tests requiring Tarantool are located in tarantool_test.go , let's benchmarks will lead the logic too.
Existing benchmarks are no longer relevant - no one knows what they're testing. Instead, benchmarks have been added for the following metrics: * Number of allocations. * RPS on synchronous and asynchronous modes.
a95a5cd    to
    334beab      
    Compare
  
    It should help to fix flaky tests on CI.
It seems that a connection's file descriptor is not always fully ready after the connection is created. The hack helps to avoid the "bad file descriptor" flaky errors for the test.
f6fd089    to
    683d1e4      
    Compare
  
    
Existing benchmarks are no longer relevant - no one knows what they're testing. Instead, benchmarks have been added for the following metrics:
The pull request also attempts to fix flaky tests found.