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

Bootstrapping #31

Closed
wants to merge 0 commits into from
Closed

Conversation

bolomcs50
Copy link

This PR:

  • Introduces bootstrapping, by caching API responses to a file whose name is specified through the new builder method cacheFileName().
  • Makes Context a struct, resolving an ambiguity where it was sometimes referred to as a class.
  • Renamed the CMake flag ENABLE_TESTING to make it project-specific, making it easier to use the library in another project.

@aruizs I hope this is ok for you, pls let me know if you see something wrong

@aruizs
Copy link
Owner

aruizs commented Feb 6, 2024

Thanks for your contribution @bolomcs50, could you add any test for this feature? Thanks.

@bolomcs50
Copy link
Author

bolomcs50 commented Feb 6, 2024

@aruizs I modified the existing tests to cover this mechanism too. Basically, now the parametrized tests initializes an unleashClient once to write the mock API response to the cache file and then initializes another one and pretends the API response is empty, thus it's forced to read from the file. This tests both file writing and reading.
Let me know if you think this proper.

P.S. I might have broken the GH actions when renaming the CMake flag, I think this is fixed by adding
-DUNLEASH_ENABLE_TESTING=ON
to the CMake configuration steps for all the OSs.

@bolomcs50
Copy link
Author

Hey @aruizs if it's ok with you I'd like open a PR I have ready to add support for constraint operators (essentially covering test set 13 of the specification) once this is merged.

aruizs
aruizs previously approved these changes Mar 6, 2024
@aruizs aruizs self-requested a review March 6, 2024 22:47
@aruizs aruizs dismissed their stale review March 6, 2024 22:49

Still reviewing.

@aruizs
Copy link
Owner

aruizs commented Mar 7, 2024

@bolomcs50 sorry for the late review. I'm working on it now. I am checking why the actions are not triggered for your pull requests.

@aruizs
Copy link
Owner

aruizs commented Apr 25, 2024

@bolomcs50 would you mind to rebase and update the PR so that the CI can be launched? Thanks.

@bolomcs50 bolomcs50 force-pushed the feature/bootstrapping branch from 6cbd6b5 to 6e86a5e Compare April 30, 2024 12:48
@bolomcs50 bolomcs50 closed this Apr 30, 2024
@bolomcs50 bolomcs50 force-pushed the feature/bootstrapping branch from 6cbd6b5 to ea61709 Compare April 30, 2024 13:11
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.

2 participants