-
Notifications
You must be signed in to change notification settings - Fork 401
Pending tests follow up #1823
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
Pending tests follow up #1823
Conversation
e4d57fc
to
8ad6506
Compare
(tendermintEnabled ? describe : xdescribe)("With WebsocketClient", () => { | ||
// don't print out WebSocket errors if marked pending | ||
const onError = globalThis.process?.env.TENDERMINT_ENABLED ? console.error : () => 0; |
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.
Good news, this ternary-operator line (and similar lines in 3 other tests) can be deleted now that xdescribe()
skips running this test code entirely, it can just use console.error
directly.
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.
true story
describe("Tendermint37Client", () => { | ||
const { url, expected } = tendermintInstances[37]; | ||
|
||
it("can connect to a given url", async () => { | ||
pendingWithoutTendermint(); | ||
|
||
(tendermintEnabled ? it : xit)("can connect to a given url", async () => { |
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.
This could all be simplified just to use xdescribe()
for all these tests instead of modifying every test to use xit()
, right?
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.
Yeah, we can probably move the check one level up.
I think all functionality that does not require a running backend should be pulled out of the Tendermint37Client into lower level code – in case something like that exists
|
||
beforeAll(async () => { | ||
if (tendermintEnabled()) { | ||
if (tendermintEnabled) { |
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.
This is basically if (true)
after this PR, so the if statement could be removed.
}); | ||
|
||
describe("connectionStatus", () => { | ||
(enabled ? describe : xdescribe)("connectionStatus", () => { |
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 for catching this. Not sure why the tests were even passing on #1809 if this test wasn't being skipped.
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.
They did not pass if no Tendermint backend is running. I got the failure locally. But in CI we always have Tendermint running.
Continuation of the work from #1809