Attempt to fix flaky API Key test#3367
Conversation
Wait until apikeys are created or invalidated before moving on in tests fixes elastic#3298
If i understand correctly, the assumption is that a read after a write to the security API might not "see" the write? Has been confirmed with the elasticsearch team that this can happen? |
|
https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-refresh.html
|
|
The system tests use a refresh interval of 250ms https://github.com/elastic/apm-server/blob/master/tests/system/config/apm-server.yml.j2#L198 |
jalvz
left a comment
There was a problem hiding this comment.
I see. I guess I didn't expect that to apply to the security API.
| password = os.getenv("ES_PASS", "changeme") | ||
| self.es_url = self.get_elasticsearch_url(self.user, password) | ||
| self.kibana_url = self.get_kibana_url() | ||
| self.base = APIKeyBase(self.es_url) |
There was a problem hiding this comment.
this is an uncommon pattern, a comment explaining why is needed would help
There was a problem hiding this comment.
It is using composition over inheritance to avoid multiple inheritance. Happy to add a comment, and change the variable name as I can see how base might be misleading.
That's a good point, I'll follow up on that. |
|
@jalvz you are right, that the referenced refresh logic doesn't apply to the creation/invalidation of API Keys. I do not see how these tests can occasionally fail under these circumstances. What do you think about still merging this PR in, as the additional |
|
Sure |
|
Merging this in then, as failing tests are unrelated. |
* Move TLS tests to separate file * Rename test_access to test_auth * Rename APIKey tests to APIKeyCommand * Move APIKey logic to dedicated class * Reuse dedicated API Key creation logic * Split commands for flaky tests for apikey command to gain more insights by waiting until apikeys are created or invalidated before moving on in tests fixes elastic#3298
* Move TLS tests to separate file * Rename test_access to test_auth * Rename APIKey tests to APIKeyCommand * Move APIKey logic to dedicated class * Reuse dedicated API Key creation logic * Split commands for flaky tests for apikey command to gain more insights by waiting until apikeys are created or invalidated before moving on in tests fixes #3298
Motivation/summary
This PR aims for fixing two flaky system tests related to API Key creation via
apikeycommand. In the current test implementation mostly the response from the command is checked, which is generally fair for testing the command. It can lead to potential issues though with following tests, when the command returned but the API Key change was not yet finished in Elasticsearch.This PR refactors current test structure a bit to move basic
wait_until_...functionality from the auth tests to the apikey command tests and make it reusable. With the changes the command tests ensure to wait until an API Key is really created or deleted in ES.Since the PR involves moving logic around, the parts where logic is only moved is kept in dedicated commits for easier review.
The PR also moves TLS related tests to a dedicated file to keep test files cleaner.
Checklist
- [ ] I have signed the Contributor License Agreement.make check-fullfor static code checks and linting)- [ ] I have made corresponding changes to the documentation- [ ] I have added tests that prove my fix is effective or that my feature works- [ ] New and existing unit tests pass locally with my changes- [ ] I have updated CHANGELOG.asciidocRelated issues
Fixes #3298