ledger-api-test-tool - Adapt for concurrent conflicts [kvl-1369] #13719
Conversation
Those are my thoughts as well, waiting for the participant team to confirm as well. But considering the purpose of the conformance tests I expect that we would have to modify a few more tests in the future to not be so strict in what they validate, and instead create other type of tests. |
| assertGrpcError( | ||
| failure, | ||
| LedgerApiErrors.Admin.ConfigurationEntryRejected, | ||
| Some("Generation mismatch"), | ||
| ) |
There was a problem hiding this comment.
Loosening the assertions here means that we don't assert returned self-service error codes, which is not fine. Why are we doing this? Is this test flaky or prohibitive to extensions?
There was a problem hiding this comment.
It's not flaky, but I find that the cost of extending and maintaining this test is more than the value it provides.s
The main change is that on our side this will now return another self-service error as well, and getting to the point where we assert 3 possible return values from a call is definitely a sign of un-maintainable tests.
This leads to the other issue where we misuse conformance tests as integration tests, with the main reasoning behind removing the assertions being the following:
- From a ledger conformance perspective I would expect that we validate that no two concurrent configurations are accepted with the same generation
- Self-service error codes are a participant only feature and we should have integration/contract tests that validate the participant output, in a more controlled environment where we can set the expectaction for a single status code instead of defaulting through multiple possible outputs
- Adding the extract assertion for a possible post-execution conflicts self-service error code to this test case feels like diluting the scope of the test a lot, having assertions that are unrelated with the scope of the test (while the error is triggered in this case, it's only because of conccurrent submission and has no relation to config managemnt itself)
I fully aggree that not testing self service error codes is not something that we should strive for, but at the same time we can't spread out the testing for this feature all throughtout the conformance tests, but it should be done in a more structured mode.
Let me know what are your thoughts on how we would proceed in the future with this type of tests, I'm also more than happy just to add a new possible error code (will at least refactor to assert only error code and not the description message)
There was a problem hiding this comment.
Self-service error codes are meant to help the app/client or system operator understand what happened when a request went bad. Aside from a couple of errors that are outside the user's control (overloaded system, serious system errors), most errors are related to a request's structure or timing. Ideally, the error code id should generally identify the problem cause and have additional information (e.g. resource, error message) specialize the error to a specific component/mechanism. So, I wouldn't say that self-service error codes are a participant-only feature.
Looking at this test: it seems we have two possible error codes for the same error condition. Ideally, we should merge them (i.e. only have a CONFIGURATION_GENERATION_MISMATCH). I will look into that.
Happy to sync further on this.
P.S. Is the error code that you need to introduce describing a new error condition or can we reuse and existing one?
There was a problem hiding this comment.
Good point that the error codes are not participant only. That makes sense.
The error code is a new one that is triggered by any post-execution conflicts.
…ultiple dars. changelog_begin changelog_end
This allows the implementation to return different errors while validating that just one submission succeeds.
nicu-da
force-pushed
the
nicuda/kvl-1369/adapt_for_post_execution_conflicts
branch
from
d84f0bf to
ff478e8
Compare
changelog_begin
changelog_end