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

C#: Make IT to manage redis. (#116) #1072

Merged

Conversation

Yury-Fridlyand
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand commented Mar 4, 2024

Run tests (IT & UT)

cd <repo root>/csharp
dotnet test

Tests with report:

dotnet test "-l:html;LogFileName=TestReport.html" --results-directory .

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.

* Make IT to run redis.


Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@Yury-Fridlyand Yury-Fridlyand added C# C# wrapper CI/CD CI/CD related labels Mar 4, 2024
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner March 4, 2024 19:47
// Nunit requires a public default constructor. These variables would be set in SetUp method.
public IntegrationTestBase()
{
STANDALONE_PORTS = new();
Copy link
Contributor

Choose a reason for hiding this comment

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

why re-init?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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

csharp/tests/Unit/Placeholder.cs Outdated Show resolved Hide resolved
private readonly string scriptDir;

// Nunit requires a public default constructor. These variables would be set in SetUp method.
public IntegrationTestBase()
Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed?

Copy link
Collaborator Author

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

Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@@ -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))
Copy link
Collaborator Author

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)
Copy link
Contributor

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)
Copy link
Contributor

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>
@Yury-Fridlyand Yury-Fridlyand merged commit 94e1186 into valkey-io:main Mar 6, 2024
9 checks passed
@Yury-Fridlyand Yury-Fridlyand deleted the csharp/integ_yuryf_it_use_script branch March 6, 2024 18:07
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024
* C#: Make IT to manage redis. (#116)

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# C# wrapper CI/CD CI/CD related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants