-
Notifications
You must be signed in to change notification settings - Fork 1k
test(site/e2e): catch missing agent defaults in fillResource
#11105
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
Conversation
53704f8
to
74c375d
Compare
site/e2e/helpers.ts
Outdated
Agent.encode(agentResource); | ||
} catch (e) { | ||
throw new Error( | ||
"agentResource encode failed, missing defaults? " + e, |
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.
One gotcha with JS errors: their default stringification method only grabs the error's name
and message
properties – the entire stack gets lost (partially because stack
technically is a non-standard property, even though it's basically added everywhere)
I would go for something like this:
throw new Error(
`agentResource encode failed, missing defaults?\n${e}\n${e.stack ?? ""}`
);
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 was looking to avoid the double stack trace (hence discarding the original one since we still create one at new Error
), but I agree that the original stack can also be useful, I'll make the change 👍🏻
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 trust Michael in terms of TypeScript 👍 .
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.
Cool. Looks good!
site/e2e/helpers.ts
Outdated
Agent.encode(agentResource); | ||
} catch (e) { | ||
throw new Error( | ||
`agentResource encode failed, missing defaults?\n${e}\n${e.stack}`, |
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.
Oh, wait – forgot there's one other issue. Since JS/TS allow you to throw any kind of value at all, TS doesn't assume that a thrown value is actually an error, and just defaults to type unknown
You'd need a type guard to prove to the compiler that the value is an error before you try accessing error-specific properties from it
try {
// Throw-able code
} catch (e) {
let errorMessage = `agentResource encode failed, missing defaults?\n${e}`;
if (e instanceof Error) {
errorMessage += `\n${e.stack}`;
}
throw new Error(errorMessage);
}
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.
Obviously, because JS, the ${e.name}: ${e.message}
is sometimes included in the stack trace (Node, etc.) and other times not (browser). Hopefully we now have a satisfying solution with the last commit. 😄
The failure in https://github.com/coder/coder/actions/runs/7143038511/job/19453662466?pr=11102#step:10:430 was not very helpful and had me scratching my head for too long.