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

chore: various gherkin improvements for e2e tests #1008

Merged

Conversation

aepfli
Copy link
Member

@aepfli aepfli commented Jul 12, 2024

This PR

  • using gherkin auto binding as described in [flagd] change e2e test to use autobinding of gherkin test #986
  • unify the location for the test harness, spec and the auto binding files within flagd-core instead of flagd and flagd-web
  • removing file operations to move the feature files before execution into a seperate folder (no need for this anymore)
  • removed step definitions and constants of e2e tests from the built package

Related Issues

Fixes #986

Follow-up Tasks

currently the path for the gherkin feature files is 'hardcoded' and not autmatically resolved relatively to the test (maybe somebody has a good idea for this)

@aepfli aepfli force-pushed the feat/gherkin_for_real branch 2 times, most recently from 794c735 to f9ba61f Compare July 12, 2024 16:38
@aepfli
Copy link
Member Author

aepfli commented Jul 12, 2024

okay, i failed here hard, i did not check the api, and the webclient and the server client do not have an overlapping api - hence that part of this improvements are not worth it. eg. the normalization witthin flagd-core. I could generate an wrapper around it, to be passed on to the stepdefinitions, but imho i do not think that this is worth it.

renovate bot and others added 4 commits October 27, 2024 14:29
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Simon Schrottner <[email protected]>
@aepfli aepfli force-pushed the feat/gherkin_for_real branch 2 times, most recently from 3c4e71a to 1e5c631 Compare October 27, 2024 13:34
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
@aepfli
Copy link
Member Author

aepfli commented Oct 27, 2024

@beeme1mr @toddbaert I think this is ready. I just have had no time to investigate why the flags changed test for flagd-web is failing. If I find the time I will investigate. If you on the other hand read this, before my next message and have more insights (maybe not implement fully) just let me know ;)

@aepfli
Copy link
Member Author

aepfli commented Oct 27, 2024

@beeme1mr @toddbaert I think this is ready. I just have had no time to investigate why the flags changed test for flagd-web is failing. If I find the time I will investigate. If you on the other hand read this, before my next message and have more insights (maybe not implement fully) just let me know ;)

Okay it seems like we are not providing a list of changed flags - as we are reloading all of them - makes sense. The question is, if it is worth checking if the flag has changed from previous calls. Or if getting the event alone is test enough

@aepfli aepfli marked this pull request as ready for review November 3, 2024 13:08
@aepfli aepfli requested a review from a team as a code owner November 3, 2024 13:08
@toddbaert
Copy link
Member

Will check this out tomorrow!

@toddbaert toddbaert changed the title chore: flagd gherkin improvements chore: use testcontainers to simplify e2e tests Nov 5, 2024
@toddbaert toddbaert changed the title chore: use testcontainers to simplify e2e tests chore: various gherkin improvements for e2e tests Nov 5, 2024
@toddbaert toddbaert self-requested a review November 5, 2024 16:53
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.

Amazing! I tested this locally a few ways, the right things are being tested and almost everything looks right.

A few minor nits I left where we are logging the wrong thing.

My only blocker is this.

aepfli and others added 3 commits November 6, 2024 10:54
Co-authored-by: Todd Baert <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Co-authored-by: Todd Baert <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

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

Looks good but please address the linting issues before merging. Nice job!

@aepfli aepfli requested a review from toddbaert November 6, 2024 18:44
@toddbaert
Copy link
Member

Amazing!

@toddbaert toddbaert merged commit 40abd8e into open-feature:main Nov 6, 2024
6 checks passed
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.

[flagd] change e2e test to use autobinding of gherkin test
3 participants