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

fix: use in memory provider for e2e suites #740

Conversation

znonthedev
Copy link
Member

This PR

Removes setup using Flagd Provider and uses In-Memory Provider

Fixes #712

@znonthedev znonthedev requested a review from a team as a code owner December 29, 2023 18:04
@znonthedev znonthedev force-pushed the fix/use-in-memory-provider-for-e2e-suites branch from 650eedc to 66c833f Compare December 29, 2023 18:05
@znonthedev
Copy link
Member Author

I had issues running the e2e locally

Running one project: client-e2e
 FAIL   client-e2e  packages/client/e2e/step-definitions/evaluation.spec.ts
  ● Test suite failed to run

    packages/client/e2e/step-definitions/evaluation.spec.ts:10:63 - error TS2307: Cannot find module '../..' or its corresponding type declarations.

    10 import { OpenFeature, ProviderEvents, InMemoryProvider } from '../..';

I saw a PR related to this #471

I changed the path locally to '../../src/' to get it working

Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

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

Hey @znonthedev,
thanks for submitting that change, this looks good!

The thing left would be to remove the flagd-testbed from the GitHub Action.

https://github.com/open-feature/js-sdk/blob/main/.github/workflows/pr-checks.yaml#L68-L72

@toddbaert toddbaert self-requested a review January 2, 2024 14:52
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Thanks @znonthedev ! This is a stop forward. The e2e tests are still failing though because you'll need to set up the InMemoryProvider to have the expected flags (you can do this in the constructor).

FAIL server-e2e packages/server/e2e/step-definitions/evaluation.spec.ts
  Flag evaluation
    ✕ Resolves boolean value (9 ms)
    ✕ Resolves string value (2 ms)
    ✕ Resolves integer value (1 ms)
    ✕ Resolves float value
    ✕ Resolves object value
    ✕ Resolves boolean details (1 ms)
    ✕ Resolves string details (1 ms)
    ✕ Resolves integer details
    ✕ Resolves float details
    ✕ Resolves object details (1 ms)
    ✕ Resolves based on context (1 ms)
    ✓ Flag not found (1 ms)
    ✕ Type error (2 ms)

The flagd test-bed has these flags configured. All the flags are defined in the gherkin suite here. The setup would like something like this (I've only included the first flag)

    OpenFeature.setProvider(
      new InMemoryProvider({
        'boolean-flag': {
          variants: {
            on: true,
            off: false,
          },
          disabled: false,
          defaultVariant: 'on',
        },
        // ...other flags...
      }),
    );

@znonthedev
Copy link
Member Author

@lukas-reining @toddbaert . Okay will make the necessary changes

@znonthedev znonthedev force-pushed the fix/use-in-memory-provider-for-e2e-suites branch from 84c9786 to 3bbfb6d Compare January 3, 2024 15:36
@znonthedev
Copy link
Member Author

@lukas-reining @toddbaert I have updated the PR with the requested changes. Please review. Why don't we remove the workflow and docker file related to flagd-testbed? I assume we don't need it anymore

@toddbaert
Copy link
Member

toddbaert commented Jan 3, 2024

Why don't we remove the workflow and docker file related to flagd-testbed? I assume we don't need it anymore

Do you mean the service in the workflow (I assume so, since you removed it)? If so, yes! That's what @lukas-reining was suggesting as well, I think.

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

@znonthedev Great work! everything passes, I just have a couple small nits, then I'm ready to approve.

Would you like an invite to the org? It comes with no obligation, but it's the first step if you'd like to be more involved.

@znonthedev znonthedev force-pushed the fix/use-in-memory-provider-for-e2e-suites branch from 43f8276 to e367d0d Compare January 3, 2024 20:16
@znonthedev
Copy link
Member Author

znonthedev commented Jan 3, 2024

Do you mean the service in the workflow (I assume so, since you removed it)? If so, yes! That's what @lukas-reining was suggesting as well, I think.

Not only the service but also the files within the test-harness submodule. I might be missing some context here. Are we using the flagd-testbed container for testing other SDKs as well?

@znonthedev
Copy link
Member Author

znonthedev commented Jan 3, 2024

@znonthedev Great work! everything passes, I just have a couple small nits, then I'm ready to approve.

Would you like an invite to the org? It comes with no obligation, but it's the first step if you'd like to be more involved.

@toddbaert Thanks a lot for the positive feedback! I'm glad everything is passing smoothly. I appreciate the opportunity to contribute.
I'd be honoured to receive an invite to the org! While my availability may vary, I'm eager to contribute more and be part of the team.
Looking forward to the next steps, and thanks again for the opportunity!

@toddbaert
Copy link
Member

toddbaert commented Jan 3, 2024

Not only the service but also the files within the test-harness submodule. I might be missing some context here.

I'm not 100% sure whether we need that submodule now, actually. I will take a quick look and follow up. Good catch...

Are we using the flagd-testbed container for testing other SDKs as well?

The flagd-testbed containers (and the associated gherkin suites) are used in tons of tests across the org, but they should all be used for testing flagd or flagd-providers. We have been trying to disentangle the actual SDKs from these flagd dependencies (mostly by using the in-memory providers as you did here).

@znonthedev
Copy link
Member Author

@toddbaert Okay, then I think no changes are needed for the shared test harness files. However, since we removed the flagd-testbed dependency in this repo, we could remove the submodule itself from this repo. I may be wrong.

@beeme1mr
Copy link
Member

beeme1mr commented Jan 5, 2024

@znonthedev Todd won't be back until Monday. I'm going to leave this open until he has a chance to approve it. I think we should be good but I would like to confirm with him. Thanks for your effort with this.

@toddbaert
Copy link
Member

So the evaluation.feature gherkin file is currently being supplied from the test-harness module, but again, in our efforts to remove all flagd-related dependencies from the SDKs, this gherkin is now available in the spec (with a few minor changes).

I've made a new issue to replace the test-harness submodule with a new spec submodule that will supply this gherkin.

@toddbaert toddbaert self-requested a review January 5, 2024 17:34
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Approved see comment here about submodule.

@beeme1mr @lukas-reining feel free to merge.

@lukas-reining lukas-reining added this pull request to the merge queue Jan 5, 2024
Merged via the queue into open-feature:main with commit 696bf4a Jan 5, 2024
8 checks passed
toddbaert added a commit that referenced this pull request Jan 8, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.9.0](server-sdk-v1.8.0...server-sdk-v1.9.0)
(2024-01-08)


### 🐛 Bug Fixes

* use in memory provider for e2e suites
([#740](#740))
([696bf4a](696bf4a))


### 🧹 Chore

* **main:** release core 0.0.22
([#745](#745))
([a0cc855](a0cc855))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Signed-off-by: OpenFeature Bot <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Todd Baert <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Jan 8, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.4.9](web-sdk-v0.4.8...web-sdk-v0.4.9)
(2024-01-08)


### ✨ New Features

* add named provider metadata accessor
([#715](#715))
([23d14aa](23d14aa))
* add PROVIDER_CONTEXT_CHANGED event (web-sdk only)
([#731](#731))
([7906bbe](7906bbe))


### 🐛 Bug Fixes

* use in memory provider for e2e suites
([#740](#740))
([696bf4a](696bf4a))


### 🧹 Chore

* **main:** release core 0.0.21
([#720](#720))
([7d1aca4](7d1aca4))
* **main:** release core 0.0.22
([#745](#745))
([a0cc855](a0cc855))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Signed-off-by: OpenFeature Bot <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Todd Baert <[email protected]>
toddbaert added a commit that referenced this pull request Jan 11, 2024
## This PR
Removes setup using Flagd Provider and uses In-Memory Provider

Fixes #712

---------

Signed-off-by: Mohamed V J <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Co-authored-by: Todd Baert <[email protected]>
toddbaert added a commit that referenced this pull request Jan 11, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.9.0](server-sdk-v1.8.0...server-sdk-v1.9.0)
(2024-01-08)


### 🐛 Bug Fixes

* use in memory provider for e2e suites
([#740](#740))
([696bf4a](696bf4a))


### 🧹 Chore

* **main:** release core 0.0.22
([#745](#745))
([a0cc855](a0cc855))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Signed-off-by: OpenFeature Bot <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Todd Baert <[email protected]>
toddbaert added a commit that referenced this pull request Jan 11, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.4.9](web-sdk-v0.4.8...web-sdk-v0.4.9)
(2024-01-08)


### ✨ New Features

* add named provider metadata accessor
([#715](#715))
([23d14aa](23d14aa))
* add PROVIDER_CONTEXT_CHANGED event (web-sdk only)
([#731](#731))
([7906bbe](7906bbe))


### 🐛 Bug Fixes

* use in memory provider for e2e suites
([#740](#740))
([696bf4a](696bf4a))


### 🧹 Chore

* **main:** release core 0.0.21
([#720](#720))
([7d1aca4](7d1aca4))
* **main:** release core 0.0.22
([#745](#745))
([a0cc855](a0cc855))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Signed-off-by: OpenFeature Bot <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Todd Baert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use in-memory provider for e2e suites
4 participants