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

Improved tests for statestore - redis #1264

Merged
merged 6 commits into from Oct 13, 2020

Conversation

akremsa
Copy link

@akremsa akremsa commented Sep 23, 2020

What this PR does / Why we need it:
I've noticed that redis.go has low unit test coverage and decided to improve that.

Before:

go test -coverprofile=before.out
PASS
coverage: 52.3% of statements
ok  	open-match.dev/open-match/internal/statestore	1.394s

After:

go test -coverprofile=after.out
PASS
coverage: 84.8% of statements
ok  	open-match.dev/open-match/internal/statestore	1.007s

@akremsa akremsa force-pushed the redis_tests branch 3 times, most recently from f187183 to 3d761bf Compare September 24, 2020 13:16
@akremsa
Copy link
Author

akremsa commented Oct 2, 2020

@Laremere could you please take a look?

@akremsa akremsa force-pushed the redis_tests branch 2 times, most recently from ff4deae to 319125e Compare October 5, 2020 12:09
test fix


fix typo
minor


fix


fix
Copy link
Collaborator

@aLekSer aLekSer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me.

Copy link

@Laremere Laremere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was already reasonably good coverage of this through the e2e tests (unfortunately coverage doesn't work across packages.) This seems like enough of a positive, and don't want to waste your work, so approving.
I think in the future if there are instances where these tests need to change and coverage is handled by e2e tests, I'll be fine with PR authors either fixing this test, or removing test cases here (again, as long as it's also covered well by e2e.)

@Laremere Laremere merged commit 6f05e52 into googleforgames:master Oct 13, 2020
@Laremere
Copy link

Thanks for the PR!

@syntxerror syntxerror added this to the v1.1.0 milestone Nov 18, 2020
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.

None yet

6 participants