search

open-feature / js-sdk-contrib

OpenFeature Providers and Hooks for JavaScript
https://openfeature.dev
Apache License 2.0
33 stars 33 forks source link

chore: removing build dependencies and using testcontainers for container spin up #982

Closed aepfli closed 1 month ago

aepfli commented 1 month ago

This PR

Removing the need for local dependencies of buf and the need to run docker compose to run e2e tests.

The Docker container will automatically start, and buf files will be loaded via npm from buf.build as generated SDKs.

I tried to do the same thing for flagd, but the generated buf files via https://github.com/stephenh/ts-proto are not delivered as SDKs from buf.build. Hence, I can't remove the dependency on buf. If we finalize this approach, I will add the test-container execution.

to Verify run nx e2e providers-flagd-web locally, it should actually run without any docker spin ups

toddbaert commented 1 month ago

This is cool!

toddbaert commented 1 month ago

Will this clash with the CI's services? Would we need to disable those?

aepfli commented 1 month ago

Will this clash with the CI's services? Would we need to disable those?

it will not clash with ci services, and if we use testcontainers everywhere we should not need to have this ci services anymore - as our test will handle the containers, and more importantly provide better port mapping out of the box - see the java reference https://github.com/open-feature/java-sdk-contrib/pull/860 - where we can actually skip the serlvices, and just configure the exposed port, and use the host port automatically (no port issues anymore)

aepfli commented 1 month ago

@toddbaert i kindly ask you to review this again.

i changed the way we build buf files, now it is just a npx command without any fancy directory changes, and file operations (just using the cli)

i removed all the setups for the e2e tests and added dedicated jest wrapper tests, which will handle the spin up and tear down of the container (those tests are a good starting point for https://github.com/open-feature/js-sdk-contrib/issues/986) - due to that all previous tests are now exporting a function which is called via the wrapper tests.

it looks big, but it is not really complicated, please review if you do have time

toddbaert commented 1 month ago

@aepfli this looks excellent to me. TBH of all our gherkin suites, I was the least happy with this one, this seems like a huge improvement. Thanks a lot. I left a few nits, and I want to check one or two things locally, but I think I'll approve soon.

toddbaert commented 1 month ago

I've marked this as a chore, since it's only a "feature" for developers, not for the users of the flagd/flagd-web providers.