elasticsearch
elasticsearch copied to clipboard
elastic
Transform Health API YAML test to Java REST test
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
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 - match: { status: "green" } to - is_true: status. We have tests that check the logic of the health indicators in detail but we are lacking a bwc test.
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 is_true instead of match and I agree that the YAML test has more value from a BWC POV.
Regarding taking it further to other indicators and adding more assertions, are you suggesting just adding assertions for the status of the other indicators? Or are you suggesting adding assertions for other parts of the response (e.g. impacts, details, diagnoses, etc.)? I'm on board with the former but I think the latter has quite some overlap with the unit tests that we already have.
Also, if we were to add assertions for the status of each indicator, it would probably make most sense to do so in individual (YAML) tests. Otherwise, if we were to add them all in one test, we'd have to bump the minimum version of that test every time we add a new indicator and thus lose significant value in terms of BWC.
What do you think?
Regarding taking it further to other indicators and adding more assertions, are you suggesting just adding assertions for the
statusof the other indicators? Or are you suggesting adding assertions for other parts of the response (e.g. impacts, details, diagnoses, etc.)? I'm on board with the former but I think the latter has quite some overlap with the unit tests that we already have.
Also, if we were to add assertions for the
statusof each indicator, it would probably make most sense to do so in individual (YAML) tests. Otherwise, if we were to add them all in one test, we'd have to bump the minimum version of that test every time we add a new indicator and thus lose significant value in terms of BWC.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 https://github.com/elastic/elasticsearch/pull/108811