test(e2e): Update e2e tests to support newer spec submodule

[kylejuliandev]

This PR

Add code to handle the new scenarios covered in the specification gherkin files. Some of the scenarios are skipped as the core SDK does not have support for it yet, for example like disabled InMemoryProvider flags.

Related Issues

Notes

Follow-up Tasks

How to test

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.89%. Comparing base (dcb4c17) to head (4c43260).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #604   +/-   ##
=======================================
  Coverage   89.89%   89.89%           
=======================================
  Files          77       77           
  Lines        3168     3168           
  Branches      364      364           
=======================================
  Hits         2848     2848           
  Misses        251      251           
  Partials       69       69           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kylejuliandev]

/gemini review

[gemini-code-assist]

Code Review

This pull request makes significant progress in expanding the E2E test suite to cover new scenarios from the specification's gherkin files. The introduction of test-flags.json is a good step towards data-driven testing. However, a major concern is that the test data from this JSON file is manually duplicated in C# code within BaseStepDefinitions.cs, which undermines the benefit of the JSON file and creates a maintenance challenge. Additionally, there are several opportunities to refactor duplicated code in the new step definition files, which would improve code quality and maintainability. My review includes specific suggestions to address these points.

[chrfwow]

Why do we need this twice?

[askpt]

As far as I remember there were a couple of steps where the type was written as Boolean and boolean. So both scenarios, even they are the "same" code wise, we need to write twice. Correct me if I'm wrong @kylejuliandev

[kylejuliandev]

Yeah exactly that. Reqnroll is case sensitive when it comes to step definitions, so I needed to declare the permutations (upper and lower case, as the gherkin files have a mix)

[aepfli]

sidequestion, can we also handle this like we do it in java/python etc. and create a mapping on the first "type"-string? like

[Given(@"a "(.*)"-flag with key ""(.*)"" and a fallback value ""(.*)""")]
public void GivenAFlagWithKeyAndADefaultValue(string type, string key, string defaultType)
{
    var flagType = ComputeFlagType(type);
    var flagState = new FlagState(key, defaultType, flagType);
    this.State.Flag = flagState;
}

and ComputeFlagType could take the string, lowercase it, and handle this like a switch case?

but anyways, we should normalize the gherkin files, and create a ticket for this, or pr, to make everything capitalized, i think it is just the "hooks.feature"

[kylejuliandev]

Fixed in 56bf219

I needed a seperate StepArgumentTransformation to handle the permutations of <flagtype>-flag, but this is definitley much more readable

[aepfli]

I am happy to see, that we are moving further into the gherkin spec direction. i left some nits, i think this move to the json file, and the new format for test steps, helps us tremendiously to ensure compliance between sdks. thank you for this contribution

[aepfli]

sidequestion, can we also handle this like we do it in java/python etc. and create a mapping on the first "type"-string? like

[Given(@"a "(.*)"-flag with key ""(.*)"" and a fallback value ""(.*)""")]
public void GivenAFlagWithKeyAndADefaultValue(string type, string key, string defaultType)
{
    var flagType = ComputeFlagType(type);
    var flagState = new FlagState(key, defaultType, flagType);
    this.State.Flag = flagState;
}

and ComputeFlagType could take the string, lowercase it, and handle this like a switch case?

but anyways, we should normalize the gherkin files, and create a ticket for this, or pr, to make everything capitalized, i think it is just the "hooks.feature"

[aepfli]

those are part of the deprecated feature files, annotated with @deprecated-tag. As we are not planning to support them in the future, and they are covered with other newer gherkin tests, should we care about those, or should we simply ignore those steps for now?

[kylejuliandev]

Removed these in d0e3580. I think when I was trying to get the e2e tests initially updated I went through and added all the step definitions. I suspect I added these before I added the exclusion on @deprecated.

[aepfli]

In other languages, we started to separate steps based on their usage, rather their file. so we can reuse them for other files.

We try to generate reusable definitions for all of those steps, and reuse those steps for all gherkin files. This separation also makes it easier to maintain and debug, as it is not feature/gherkin file based, but implementation based. maybe it is worth also following this approach here.

so far we established this kind of separation:

• providerSteps - for handling provider related topic
• flagSteps - evaluation and flag related
• contextSteps - context related
• metadataSteps - metadata related
• hookSteps - hook related

the background is, in the long run, if we do this properly, these steps could theoretically also be used by provider implementations. the only thing they need to do is providing own providerSteps/Mappings, and they could test if they are openFeature compliant and everything works as intended. (really long run idea)

[kylejuliandev]

I've attempted to align the step definition files to the types of separation. I've left the older EvaluationStepDefinitions file for now, this maps to one of the deprecated feature files. f5470eb has the changes for this.