-
Notifications
You must be signed in to change notification settings - Fork 59
Avoid scan rate limiting in TokenStandardCliTestDataTimeBasedIntegrationTest
#3635
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
Avoid scan rate limiting in TokenStandardCliTestDataTimeBasedIntegrationTest
#3635
Conversation
…tionTest` Fixes DACH-NY/cn-test-failures#6839 [ci] Signed-off-by: Martin Florian <martin.florian@digitalasset.com>
moritzkiefer-da
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
...ralizedtrust/splice/integration/tests/TokenStandardCliTestDataTimeBasedIntegrationTest.scala
Outdated
Show resolved
Hide resolved
| def replaceStringsInJson(viewValue: Json) = { | ||
| val current = viewValue.spaces2SortKeys | ||
| val amuletRulesId = sv1ScanBackend.getAmuletRules().contractId.contractId | ||
| val amuletRulesId = eventually()(sv1ScanBackend.getAmuletRules().contractId.contractId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the eventually for? If it's for handling the command failure from it not existing (not sure why it would not exist?) don't you need an eventuallySucceeds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I meant to comment on this line before marking ready, sorry...
what is the eventually for?
Just a cheap extra safety against flakes... this is the thing that failed previously. I don't care here if scan has a random hiccup.
don't you need an eventuallySucceeds
Yes, thanks for catching that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original issue was that this call failed due to rate limiting. Why is only this call being rate limited? Can we cache the amulet rules to reduce the amount of calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a dumb call from out of the test itself... I wouldn't overthink this tbh; we're not testing rate limiting here and the thing that is causing too much load on scan is not part of our app logic; just the test logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we could implement caching inside of the test but the gain should just be a minor test duration improvement... doesn't seem worth it to me.
…ation/tests/TokenStandardCliTestDataTimeBasedIntegrationTest.scala Co-authored-by: moritzkiefer-da <45630097+moritzkiefer-da@users.noreply.github.com> Signed-off-by: Martin Florian <martin.florian@digitalasset.com>
[static] Signed-off-by: Martin Florian <martin.florian@digitalasset.com>
[force] Signed-off-by: Martin Florian <martin.florian@digitalasset.com>
Fixes https://github.com/DACH-NY/cn-test-failures/issues/6839
[ci]
Signed-off-by: Martin Florian martin.florian@digitalasset.com