-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Transform Health API YAML test to Java REST test #108666
Conversation
This test failed occasionally due to the health node not having received the health info from all nodes yet, resulting in an "unknown" status. In a Java test, we do have the option to retry.
Pinging @elastic/es-data-management (Team:Data Management) |
Before we move forward with the detailed review of this change, I would like to challenge this transfer from yaml to java. If we take a look at this test, it is a very basic happy flow API test. My assumption here, is that this is a meant to test the structure of the response and that it remains bwc. For these reasons, I think the value of this tests lies on the fact that it is a yaml test. If we weaken one assertion, it might still be more valuable than the equivalent java rest test. Namely, if we switched the So, if this is an satisfactory proposal, we could take it further and extend it to other indicators. For each indicator we can express what we are certain about in more precise assertions and for the rest with weaker ones. For example, for the disk indicator it could check that it's there and there is a status but not that it's green. On the other hand, for the shards availability we can check that it is indeed green and so on. Thoughts? |
@gmarouli Thank you for your input and suggestions! I like your idea of doing Regarding taking it further to other indicators and adding more assertions, are you suggesting just adding assertions for the Also, if we were to add assertions for the What do you think? |
Good questions, I think I had all of it in mind. If this is supposed to test the structure of our API response then I think it should check all of them. I see your concern about bumping the version, but we do not have that active development to worry about this I think. I believe it would pay off in the long run. If we see that it becomes a problem we could indeed split it into different tests too. We could split the assertions in different tests, still request all the indicators but each test would test only one. I am worrying that it might be too many tests, on the other hand if there is a failure it will be easier to debug :). So, no strong opinions here. |
Closing in favor of a different approach, see #108811 |
This test failed occasionally due to the health node not having received the health info from all nodes yet, resulting in an "unknown" status. In a Java test, we do have the option to retry.
Fixes #107796