Fix cloud integration tests #535
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| const value = row.json() | ||
| if (isException(value)) { | ||
| throw parseError(value.exception) | ||
| } |
There was a problem hiding this comment.
I'm not sure how did the tests work before this @slvrtrn, likely I'm fixing the symptom not the root case here.
There was a problem hiding this comment.
depends on the version of ClickHouse you are testing with
this should work after 25.11: https://github.com/ClickHouse/clickhouse-js/pull/535/changes#diff-e515192c535cccfaf415f4ff430d266ec9fdfef2d6262aa96af3fdb312543310R170-R177
cause json still calls stream under the hood
There was a problem hiding this comment.
Yep, this is clear, thanks. Do you think the isException() is appropriate here?
I'm thinking of creating a compatibility matrix that will allow to know ahead of time from the config options and CH version if this isException() is really required for each row. WDYT?
There was a problem hiding this comment.
Also, the tests broke without the JS client changing code, it seems like the updated CH version might have changed something on the server side thus the explicit config option.
There was a problem hiding this comment.
it seems like the updated CH version might have changed something on the server side thus the explicit config option.
What version do we test against? Why don't we see such problem against HEAD or latest?
There was a problem hiding this comment.
Good question. If you folks don't know from the top of your heads, I'll dive deeper into what caused the degradation.
Summary
A few weeks ago the cloud integration tests started failing. I'm guessing this is because of the gradual rollout of the new exception handling mechanism.
This PR makes some changes that fix the tests.