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

Run tests on all (two) implementations #56

Merged
merged 3 commits into from Jan 19, 2021
Merged

Run tests on all (two) implementations #56

merged 3 commits into from Jan 19, 2021

Conversation

rabbbit
Copy link
Contributor

@rabbbit rabbbit commented Jan 3, 2021

The test runner will simply execute all tests on all implementations.

This is so that we can:

  • make sure both implementations are equivalent
  • start adding more implementations
  • have proper test coverage, yay.

The diff is slightly bigger than the actual changes:

  • I'm modifying the tests to run in the same package as the code,
    so that I have access to each of the implementations.
  • I'm renaming runner to testRunner since benchmark defines it's
    own runner.

I considered somehow keeping the ratelimit_test package,
and also having a separate ratelimit tests to just test
implementations but that feels too complicated for the benefit - lets
keep it simple and just have a single set of tests.

We could probably also make the benchmark tests re-use the same test runner,
but that's a separate follow up.

The test runner will simply execute all tests on all implementations.

This is so that we can:
- make sure both implementations are equivalent
- start adding more implementations
- have proper test coverage, yay.

The diff is slightly bigger than the actual changes:
- I'm modifying the tests to run in the same package as the code,
  so that I have access to each of the implementations.
- I'm renaming `runner` to `testRunner` since benchmark defines it's
  own runner.

I considered somehow keeping the `ratelimit_test` package,
and also having a separate `ratelimit` tests to just test
implementations  but that feels too complicated for the benefit - lets
keep it simple and just have a single set of tests.

We could probably also make the benchmark tests re-use the same test runner,
but that's a separate follow up.
@codecov
Copy link

codecov bot commented Jan 3, 2021

Codecov Report

Merging #56 (d0a8f9a) into master (4fc173c) will increase coverage by 34.92%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           master       #56       +/-   ##
============================================
+ Coverage   65.07%   100.00%   +34.92%     
============================================
  Files           3         3               
  Lines          63        63               
============================================
+ Hits           41        63       +22     
+ Misses         22         0       -22     
Impacted Files Coverage Δ
limiter_mutexbased.go 100.00% <0.00%> (+100.00%) ⬆️

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 4fc173c...d0a8f9a. Read the comment docs.

@rabbbit rabbbit requested a review from cinchurge January 3, 2021 19:03
rabbbit added a commit that referenced this pull request Jan 3, 2021
#56 will bump us to 100%.
@rabbbit rabbbit mentioned this pull request Jan 3, 2021
rabbbit added a commit that referenced this pull request Jan 8, 2021
#56 will bump us to 100%.
@rabbbit rabbbit changed the title Run tests on all (both) implementations Run tests on all (two) implementations Jan 12, 2021
ratelimit_test.go Outdated Show resolved Hide resolved
ratelimit_test.go Outdated Show resolved Hide resolved
@rabbbit rabbbit merged commit 688a9ec into master Jan 19, 2021
@rabbbit rabbbit deleted the pawel/both branch January 19, 2021 02:25
@rabbbit
Copy link
Contributor Author

rabbbit commented Jan 19, 2021

thanks @abhinav.

@rabbbit rabbbit mentioned this pull request Jan 19, 2021
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

2 participants