Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,18 +1,57 @@
using System;
using System.Collections.Generic;
using OpenFeature.Contrib.Providers.Flagd.E2e.Common.Utils;
using Reqnroll;
using Xunit;

namespace OpenFeature.Contrib.Providers.Flagd.E2e.Common.Steps;

#nullable enable

[Binding]
public class ConfigSteps
{
private readonly State _state;

private FlagdConfig? _config;

private static readonly HashSet<string> _environmentVariables =
[
"FLAGD_RESOLVER",
"FLAGD_HOST",
"FLAGD_PORT",
"FLAGD_TARGET_URI",
"FLAGD_TLS",
"FLAGD_SOCKET_PATH",
"FLAGD_SERVER_CERT_PATH",
"FLAGD_DEADLINE_MS",
"FLAGD_STREAM_DEADLINE_MS",
"FLAGD_RETRY_BACKOFF_MS",
"FLAGD_RETRY_BACKOFF_MAX_MS",
"FLAGD_RETRY_GRACE_PERIOD",
"FLAGD_KEEP_ALIVE_TIME_MS",
"FLAGD_CACHE",
"FLAGD_MAX_CACHE_SIZE",
"FLAGD_SOURCE_SELECTOR",
"FLAGD_OFFLINE_FLAG_SOURCE_PATH",
"FLAGD_OFFLINE_POLL_MS",
"FLAGD_FATAL_STATUS_CODES"
];

public ConfigSteps(State state)
{
this._state = state;
}

[BeforeScenario]
public void BeforeScenario()
{
foreach (var envVar in _environmentVariables)
{
Environment.SetEnvironmentVariable(envVar, null);
}
}

[Given("an option {string} of type {string} with value {string}")]
public void GivenAnOptionOfTypeWithValue(string option, string type, string value)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The type parameter is not used in this method. The implementation relies on the option name to determine how to parse the value. This makes the type parameter redundant. If the type parameter is part of a shared test specification that cannot be changed, consider adding a comment explaining why it's unused. Otherwise, it should be used for parsing or removed from the step definition.

{
Expand All @@ -21,14 +60,157 @@ public void GivenAnOptionOfTypeWithValue(string option, string type, string valu
this._state.FlagdConfig = FlagdConfig.Builder();
}

if (option == "cache")
Skip.If(option == "deadlineMs", "DeadlineMs is not supported.");
Skip.If(option == "fatalStatusCodes", "FatalStatusCodes is not supported.");
Skip.If(option == "targetUri", "TargetUri is not supported.");
Skip.If(option == "providerId", "ProviderId is not supported.");
Skip.If(option == "offlineFlagSourcePath", "OfflineFlagSourcePath is not supported.");
Skip.If(option == "offlinePollIntervalMs", "OfflinePollIntervalMs is not supported.");
Skip.If(option == "streamDeadlineMs", "StreamDeadlineMs is not supported.");
Skip.If(option == "keepAliveTime", "KeepAliveTime is not supported.");
Skip.If(option == "retryBackoffMs", "RetryBackoffMs is not supported.");
Skip.If(option == "retryBackoffMaxMs", "RetryBackoffMaxMs is not supported.");
Skip.If(option == "retryGracePeriod", "RetryGracePeriod is not supported.");
Comment on lines +63 to +73
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This block of Skip.If calls is quite repetitive and could be hard to maintain. Consider creating a static readonly HashSet<string> containing all unsupported options and then checking against it with a single Skip.If call. This would make the code cleaner and more efficient. A similar block of code exists on lines 144-147 that could also be refactored.


switch (option)
{
var enabled = value == "enabled";
this._state.FlagdConfig = this._state.FlagdConfig.WithCache(enabled);
case "host":
{
this._state.FlagdConfig = this._state.FlagdConfig.WithHost(value);
break;
}
case "port":
{
var port = int.Parse(value);
this._state.FlagdConfig = this._state.FlagdConfig.WithPort(port);
break;
}
case "tls":
{
var useTls = bool.Parse(value);
this._state.FlagdConfig = this._state.FlagdConfig.WithTls(useTls);
break;
}
case "certPath":
{
this._state.FlagdConfig = this._state.FlagdConfig.WithCertificatePath(value);
break;
}
case "socketPath":
{
this._state.FlagdConfig = this._state.FlagdConfig.WithSocketPath(value);
break;
}
case "cache":
{
var enabled = value == "enabled";
this._state.FlagdConfig = this._state.FlagdConfig.WithCache(enabled);
break;
}
case "maxCacheSize":
{
var maxCacheSize = int.Parse(value);
this._state.FlagdConfig = this._state.FlagdConfig.WithMaxCacheSize(maxCacheSize);
break;
}
case "selector":
{
this._state.FlagdConfig = this._state.FlagdConfig.WithSourceSelector(value);
break;
}
case "resolver":
{
var resolverType = value == "rpc" ? ResolverType.RPC : ResolverType.IN_PROCESS;
this._state.FlagdConfig = this._state.FlagdConfig.WithResolverType(resolverType);
break;
}
default:
break;
}
else if (option == "selector")
}

[When("a config was initialized")]
public void WhenAConfigWasInitialized()
{
var flagdConfigBuilder = this._state.FlagdConfig ?? FlagdConfig.Builder();
this._config = flagdConfigBuilder.Build();

Assert.NotNull(this._state);
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The _state field is injected via constructor dependency injection by Reqnroll and is initialized in the constructor. It will not be null at this point, so this assertion is redundant and can be removed.

}

[Then("the option {string} of type {string} should have the value {string}")]
public void ThenTheOptionOfTypeShouldHaveTheValue(string option, string type, string value)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The type parameter is not used in this method, similar to GivenAnOptionOfTypeWithValue. The implementation relies on the option name to determine how to parse and compare the value. This makes the type parameter redundant. If the type parameter is part of a shared test specification that cannot be changed, consider adding a comment explaining why it's unused. Otherwise, it should be used for parsing/comparison or removed from the step definition.

{
Skip.If(option == "deadlineMs", "DeadlineMs is not supported.");
Skip.If(option == "fatalStatusCodes", "FatalStatusCodes is not supported.");
Skip.If(option == "targetUri", "TargetUri is not supported.");
Skip.If(option == "providerId", "ProviderId is not supported.");

switch (option)
{
this._state.FlagdConfig = this._state.FlagdConfig.WithSourceSelector(value);
case "resolver":
{
var expected = value.ToLower();
var actual = this._config!.ResolverType == ResolverType.RPC ? "rpc" : "in-process";
Assert.Equal(expected, actual);
break;
}
case "host":
{
var expected = value;
var actual = this._config!.Host;
Assert.Equal(expected, actual);
break;
}
case "port":
{
var expected = int.Parse(value);
var actual = this._config!.Port;
Assert.Equal(expected, actual);
break;
}
case "tls":
{
var expected = bool.Parse(value);
var actual = this._config!.UseTls;
Assert.Equal(expected, actual);
break;
}
case "certPath":
{
var expected = value == "null" ? string.Empty : value;
var actual = this._config!.CertificatePath;
Assert.Equal(expected, actual);
break;
}
case "socketPath":
{
var expected = value == "null" ? string.Empty : value;
var actual = this._config!.SocketPath;
Assert.Equal(expected, actual);
break;
}
case "selector":
{
var expected = value == "null" ? string.Empty : value;
var actual = this._config!.SourceSelector;
Assert.Equal(expected, actual);
break;
}

default:
break;
}
}

[Then("we should have an error")]
public void ThenWeShouldHaveAnError()
{
}

[Given("an environment variable {string} with value {string}")]
public void GivenAnEnvironmentVariableWithValue(string env, string value)
{
Environment.SetEnvironmentVariable(env, value);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ public void BeforeScenario(ScenarioInfo scenarioInfo, FeatureInfo featureInfo)
var scenarioTags = scenarioInfo.Tags;
var featureTags = featureInfo.Tags;
var tags = new HashSet<string>(scenarioTags.Concat(featureTags));
Skip.If(!tags.Contains("in-process"), "Skipping scenario because it does not have required tag.");
Skip.If(!tags.Contains("in-process"), "Skipping scenario because it is not for the in-process resolver.");

// TODO: https://github.com/open-feature/dotnet-sdk-contrib/issues/478
Skip.If(tags.Contains("sync-port"), "Skipping sync-port as it is not supported.");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
<None Include="../../src/OpenFeature.Contrib.Providers.Flagd/flagd-testbed/gherkin/targeting.feature" Link="../../../Features/%(Filename)%(Extension)" DestinationFolder="../../../Features/" CopyToOutputDirectory="PreserveNewest" />
<None Include="../../src/OpenFeature.Contrib.Providers.Flagd/flagd-testbed/gherkin/selector.feature" Link="../../../Features/%(Filename)%(Extension)" DestinationFolder="../../../Features/" CopyToOutputDirectory="PreserveNewest" />
<None Include="../../src/OpenFeature.Contrib.Providers.Flagd/flagd-testbed/gherkin/contextEnrichment.feature" Link="../../../Features/%(Filename)%(Extension)" DestinationFolder="../../../Features/" CopyToOutputDirectory="PreserveNewest" />
<None Include="../../src/OpenFeature.Contrib.Providers.Flagd/flagd-testbed/gherkin/config.feature" Link="../../../Features/%(Filename)%(Extension)" DestinationFolder="../../../Features/" CopyToOutputDirectory="PreserveNewest" />
</ItemGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,6 @@ public void BeforeScenario(ScenarioInfo scenarioInfo, FeatureInfo featureInfo)
var scenarioTags = scenarioInfo.Tags;
var featureTags = featureInfo.Tags;
var tags = new HashSet<string>(scenarioTags.Concat(featureTags));
Skip.If(!tags.Contains("rpc"), "Skipping scenario because it does not have required tag.");
Skip.If(!tags.Contains("rpc"), "Skipping scenario because it is not for the rpc resolver.");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<None Include="../../src/OpenFeature.Contrib.Providers.Flagd/flagd-testbed/gherkin/evaluation.feature" Link="../../../Features/%(Filename)%(Extension)" DestinationFolder="../../../Features/" CopyToOutputDirectory="PreserveNewest" />
<None Include="../../src/OpenFeature.Contrib.Providers.Flagd/flagd-testbed/gherkin/targeting.feature" Link="../../../Features/%(Filename)%(Extension)" DestinationFolder="../../../Features/" CopyToOutputDirectory="PreserveNewest" />
<None Include="../../src/OpenFeature.Contrib.Providers.Flagd/flagd-testbed/gherkin/contextEnrichment.feature" Link="../../../Features/%(Filename)%(Extension)" DestinationFolder="../../../Features/" CopyToOutputDirectory="PreserveNewest" />
<None Include="../../src/OpenFeature.Contrib.Providers.Flagd/flagd-testbed/gherkin/config.feature" Link="../../../Features/%(Filename)%(Extension)" DestinationFolder="../../../Features/" CopyToOutputDirectory="PreserveNewest" />
</ItemGroup>

<ItemGroup>
Expand Down
Loading