mennanov / limiters

Golang rate limiters for distributed applications
https://godoc.org/github.com/mennanov/limiters
MIT License
411 stars 47 forks source link

Improve tests #36

Closed leeym closed 6 months ago

leeym commented 6 months ago

This PR tries to address a few issues to make developer experience a little bit better.

It will be easier to review this PR in split view with Hide whitespace checked. https://github.com/mennanov/limiters/pull/36/files?diff=split&w=1

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.46%. Comparing base (310e1c4) to head (ec1e605).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #36 +/- ## ======================================= Coverage 83.46% 83.46% ======================================= Files 10 10 Lines 1464 1464 ======================================= Hits 1222 1222 Misses 169 169 Partials 73 73 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mennanov commented 6 months ago

If any test fails, LimitersTestSuite.TearDownSuite will not be executed so the dynamo tables will not be deleted, and it blocks future SetupSuite as it will fail with ResourceInUseException. Delete these table first to unblock it.

Could you elaborate on this one, please? Why will LimitersTestSuite.TearDownSuite not be executed when a test fails? Also it looks like you added a code that deletes the DynamoDB table in SetupSuite - do we need to delete the table in the TearDownSuite then?

leeym commented 6 months ago

Why will LimitersTestSuite.TearDownSuite not be executed when a test fails?

Sorry for miscommunication. TearDownSuite will not be executed if SetupSuite fails immediately. Here comes the diff against the latest master to reproduce the issue.

diff --git a/limiters_test.go b/limiters_test.go
index 1e705b9..891e7d3 100644
--- a/limiters_test.go
+++ b/limiters_test.go
@@ -117,9 +117,13 @@ func (s *LimitersTestSuite) SetupSuite() {

        s.memcacheClient = memcache.New(strings.Split(os.Getenv("MEMCACHED_ADDR"), ",")...)
        s.Require().NoError(s.memcacheClient.Ping())
+       s.T().Log("FOO")
+       s.FailNow("BAR")
+       s.T().Log("BAZ")
 }

 func (s *LimitersTestSuite) TearDownSuite() {
+       s.T().Log("QUX")
        s.Assert().NoError(s.etcdClient.Close())
        s.Assert().NoError(s.redisClient.Close())
        s.Assert().NoError(DeleteTestDynamoDBTable(context.Background(), s.dynamodbClient))

I basically use s.FailNow(...) to simulate s.Require().whatever_check_that_fails_(...)

The first time you see message FOO with error BAR while there is no message BAZ and QUX, and all the following tests will fail with ResourceInUseException

it looks like you added a code that deletes the DynamoDB table in SetupSuite - do we need to delete the table in the TearDownSuite then?

I added a code that deletes the dynamo table in SetupSuite so even if the table exists it will still work, and we should still delete the table in TearDownSuite so it doesn't change the state after the tests.