chore: replace test-harness submodule with spec submodule#1359
chore: replace test-harness submodule with spec submodule#1359MridulTailor wants to merge 2 commits intoopen-feature:mainfrom
Conversation
Replace the test-harness submodule with the spec submodule to remove flagd-related dependencies and use the evaluation.feature gherkin file from the OpenFeature specification. Changes: - Remove test-harness submodule and add spec submodule - Update package.json scripts to reference spec/specification/assets/gherkin/evaluation.feature - Update CONTRIBUTING.md documentation links - Update test step definitions to match spec gherkin language changes: - 'a provider is registered with cache disabled' -> 'a stable provider' - 'default value' -> 'fallback value' in error scenarios - Rename helper function givenAnOpenfeatureClientIsRegisteredWithCacheDisabled to givenAStableProvider All e2e tests passing (13 scenarios in both server and web packages). Fixes open-feature#743 Signed-off-by: Mridul Tailor <mtailor729@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the project's testing infrastructure by integrating directly with the OpenFeature specification. It removes an external 'test-harness' dependency, reducing potential for flagd-specific coupling, and ensures that end-to-end tests are driven by the official specification's Gherkin feature file. This change improves consistency and maintainability of the testing suite. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly replaces the test-harness submodule with the spec submodule, which is a good move to reduce dependencies and align with the OpenFeature specification. The changes are well-documented in the PR description. I've identified a few areas for improvement, mainly related to code duplication in the package.json scripts and some confusing variable names in the e2e tests. My feedback aims to enhance the maintainability and clarity of the codebase.
| "e2e-server": "git submodule update --init --recursive && shx cp spec/specification/assets/gherkin/evaluation.feature packages/server/e2e/features && jest --selectProjects=server-e2e --verbose", | ||
| "e2e-web": "git submodule update --init --recursive && shx cp spec/specification/assets/gherkin/evaluation.feature packages/web/e2e/features && jest --selectProjects=web-e2e --verbose", | ||
| "e2e": "npm run e2e-server && npm run e2e-web", |
There was a problem hiding this comment.
The e2e-server and e2e-web scripts duplicate the git submodule update command and the path to evaluation.feature. This causes the submodule update to run twice when npm run e2e is executed. While the performance impact is likely minimal, this can be improved for better maintainability.
Consider extracting the preparation step into a separate script to centralize the setup logic. For example:
"scripts": {
...
"e2e:prepare": "git submodule update --init --recursive && shx cp spec/specification/assets/gherkin/evaluation.feature packages/server/e2e/features && shx cp spec/specification/assets/gherkin/evaluation.feature packages/web/e2e/features",
"e2e-server": "npm run e2e:prepare && jest --selectProjects=server-e2e --verbose",
"e2e-web": "npm run e2e:prepare && jest --selectProjects=web-e2e --verbose",
"e2e": "npm run e2e:prepare && jest --selectProjects=server-e2e,web-e2e --verbose",
...
}This would make the scripts cleaner, though it's worth noting it changes the behavior of e2e-server and e2e-web to prepare the environment for both test suites.
Replace the test-harness submodule with the spec submodule to remove flagd-related dependencies and use the evaluation.feature gherkin file from the OpenFeature specification.
This PR
This PR replaces the
test-harnesssubmodule with thespecsubmodule to remove flagd-related dependencies from the SDK and use the evaluation.feature gherkin file directly from the OpenFeature specification repository.Changes Made:
test-harnesssubmodule and addedspecsubmodulee2e-serverande2e-webscripts inpackage.jsonto referencespec/specification/assets/gherkin/evaluation.feature'a provider is registered with cache disabled'to'a stable provider''default value'to'fallback value'givenAnOpenfeatureClientIsRegisteredWithCacheDisabledtogivenAStableProviderRelated Issues
Fixes #743
Notes
@deprecatedbut is still functional and matches our test requirementsFollow-up Tasks
evaluation_v2.featureif/when it becomes the recommended testing standardHow to test
Run the e2e tests to verify the integration with the new spec submodule: