-
Notifications
You must be signed in to change notification settings - Fork 69
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
C#: Make IT to manage redis. (#116) #1072
C#: Make IT to manage redis. (#116) #1072
Conversation
* Make IT to run redis. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
// Nunit requires a public default constructor. These variables would be set in SetUp method. | ||
public IntegrationTestBase() | ||
{ | ||
STANDALONE_PORTS = new(); |
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.
why re-init?
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 following there a bad coding practice - using static members as class ones, because this class is a singletone-like.
Unfortunately, I can't make it as a singletone, because nunit requires a public constructor.
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.
Reworked that, should be better now
private readonly string scriptDir; | ||
|
||
// Nunit requires a public default constructor. These variables would be set in SetUp method. | ||
public IntegrationTestBase() |
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.
please use internal when possible.
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.
nunit requires a public default constructor https://docs.nunit.org/articles/nunit/writing-tests/attributes/setupfixture.html
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 internal
config
[OneTimeSetUp] | ||
public void Setup() | ||
{ | ||
Glide.Logger.SetLoggerConfig(Glide.Level.Info); |
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.
why was this removed?
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.
Do we need that? Logs produce too much noise and aren't helpful.
If you need logs, I'll activate them, but in IntegrationTestBase::SetUp
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.
we use info in tests by default, in order to get decent logs on our tests. they're loud, but it helps for failures in CI.
|
||
using Glide; | ||
|
||
// TODO - need to start a new redis server for each test? | ||
public class AsyncClientTests | ||
public class GetAndSet |
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.
why is this a class? what's the intended separation between test classes?
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.
No separation, I just renamed it. I assume we won't put all tests into a single class.
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.
so, again - what's the intended separation?
@@ -27,7 +22,7 @@ private async Task GetAndSetRandomValues(AsyncClient client) | |||
[Test] | |||
public async Task GetReturnsLastSet() | |||
{ | |||
using (var client = new AsyncClient("localhost", 6379, false)) | |||
using (var client = new AsyncClient("localhost", TestConfiguration.STANDALONE_PORTS[0], false)) |
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.
Do you want me to create client once for all these tests?
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
scriptDir = Path.Combine(projectDir, "..", "utils"); | ||
} | ||
|
||
public List<uint> StartRedis(bool cluster, bool tls = false, string? name = null) |
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.
internal or private
/// <summary> | ||
/// Stop <b>all</b> instances on the given <paramref name="name"/>. | ||
/// </summary> | ||
public void StopRedis(bool keepLogs, string? name = null) |
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.
same. please go over the visibility rules for all functions and fix them
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
* C#: Make IT to manage redis. (#116) Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Run tests (IT & UT)
Tests with report:
The approach is the same as for java client IT.
IntegrationTestBase
class provides API to get redis ports, redis version and start/stop a new instance.