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

added test cases for flags with empty string value #108

Conversation

MagicMarka
Copy link
Collaborator

Loosen validation to allow empty string in string-valued flag variations was added in this ticker https://linear.app/eppo/issue/FF-3794/loosen-validation-to-allow-empty-string-in-string-valued-flag

@MagicMarka MagicMarka requested a review from aarsilv January 22, 2025 07:48
@MagicMarka MagicMarka self-assigned this Jan 22, 2025
Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

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

Some typing and object property errors in here. Look at test-case-new-user-onboarding-flag.json for an example string-valued flag test file.

Also, you can try this out locally by checking out the common javascript SDK and then dropping this file in the test data directory so it is run. That is the development pattern I use.

@MagicMarka MagicMarka marked this pull request as draft January 23, 2025 15:06
@MagicMarka MagicMarka marked this pull request as ready for review January 24, 2025 08:10
Copy link
Collaborator

@typotter typotter left a comment

Choose a reason for hiding this comment

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

Thanks for working through this one with me.
You will need to rebase with main before the js-client tests will run and pass properly (to pick up my workflow fix)

I reran the test suites but the js-client seems to be stuck using the test workflow from before my fix. If you push changes to your branch, it should reload the workflow files from the remote repositories

I dug into the multiplatform test failing and found an issue in the test file that I suppose only that SDK is checking. I made a suggested change in the test file with the expected structure for you.

Thanks again! Please make sure the test suites are all passing before merging.

}
}
]
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:add newline please.

@typotter
Copy link
Collaborator

I tracked down the cause of react-native and js-client tests failing
It's an issue with a truthy check in the test harness. SDK is doing the right thing. Fixes to the tests are here:

Eppo-exp/react-native-sdk#72
Eppo-exp/js-client-sdk#165

Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

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

Thanks for iterating! Assuming these pass when you drop them into an SDK and run all the tests cases, I'd say we are good 🚀

@MagicMarka MagicMarka merged commit d46b866 into main Jan 30, 2025
34 of 39 checks passed
@MagicMarka MagicMarka deleted the marka/ff-3834-add-test-case-to-sdk-test-data-for-ff-with-empty-strings-as branch January 30, 2025 09:59
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.

3 participants