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

[testHarnes/uiSettings] create stub uiSettings client with actual state #17588

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Apr 6, 2018

Rather than create the UiSettings client in the test harness, embed the logic in the client itself so that we can persist a little bit of state like observers and defaults.

@spalger spalger added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v6.3.0 labels Apr 6, 2018
@spalger spalger requested a review from kimjoar April 6, 2018 01:35
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kimjoar
Copy link
Contributor

kimjoar commented Apr 6, 2018

Not sure I'm following why this is needed. Generally I'm not a fan of putting test code within production code unless there's a really good reason. If this is simplifying tests or something like that, it would be great to include those changes in this PR.

@spalger
Copy link
Contributor Author

spalger commented Apr 6, 2018

I guess it's mostly a precautionary measure for PRs like #17581.

That PR will be the first to utilize the new non-angular uiSettings client, and I'm mostly just worried about the way that the client will interact with mutations coming from the tests.

@ppisljar I'm not going to try to get this PR merged, but if you run into weird state issues when trying to test the new field formats registry, ping me and we can see if the changes in this PR help out.

@spalger spalger closed this Apr 6, 2018
@spalger
Copy link
Contributor Author

spalger commented Apr 6, 2018

Also, @tsullivan @sqren if you guys see weird caching/state issues in your browser tests that seem connected to the uiSettings client, please ping me so we can see if this helps.

@kimjoar
Copy link
Contributor

kimjoar commented Apr 7, 2018

Ah, I see 👍 I’m not against this if it makes tests less flaky. If we end up merging this it would be great with a comment on the code that explains why it’s put with the production code.

@spalger spalger deleted the implement/test-harness-persist-local-defaults branch August 18, 2020 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.3.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants