Propagate exception to wasm-js and js in propagateExceptionFinalResort

[murfel]

Fixes #4451

[dkhalanskyjb]

Makes sense, thanks!

[dkhalanskyjb]

Why is this needed? Please add a comment to the code.

[dkhalanskyjb]

I'd expect a more descriptive name, like testHandlingUncaughtExceptionsInNodeJs. Maybe even a documentation comment explaining what exactly we are checking here. Given how much JS code there is in this file, it's not trivial to figure out.

[murfel]

I feel like test name is too short to include a very specific descriptive name, so I've only added a comment. Do you want me to rename the test name anyway into the next best thing? (I assume with the goal of having a better name should this test fail.)

[dkhalanskyjb]

Yes, having a rough idea of what failed when you see the list of failed tests after introducing a change is the point.

[murfel]

It's just that all the info is in the class name + path (for platform), and naming this accurately would be duplicating those. Had we had a dozen tests, we wouldn't be naming them so verbosely. I've done it anyways for now.

[murfel]

Idea's internal formatter (CMD+Option+L) wants to format all js("""...""") blocks like this:

js(
    """
    globalThis.originalListeners = process.listeners('uncaughtException');
    process.removeAllListeners('uncaughtException');
"""
)

Instead of the simpler this, which is also the way it is written in docs.

js("""
    globalThis.originalListeners = process.listeners('uncaughtException');
    process.removeAllListeners('uncaughtException');
""")

Should we override the formatter, locally or globally, for this particular function?

[dkhalanskyjb]

What the IDE does is not the style used in our style guide: https://kotlinlang.org/docs/coding-conventions.html#strings Worth filing a bug, but I'm not sure who is wrong: the IDE, the style guide, or both.

The important thing, though, is for a project to have a consistent style, the specifics don't matter that much. In kotlinx.coroutines, we don't yet have a style for """ (grepping """, I see all sorts of formatting), so if you have a preference, please feel free to declare it the official kotlinx.coroutines style for """ and apply it everywhere.

[dkhalanskyjb]

Well done, thank you! I only have some minor remarks, but otherwise, this is good to go.

[dkhalanskyjb]

Yes, having a rough idea of what failed when you see the list of failed tests after introducing a change is the point.

[dkhalanskyjb]

Minor: an extra space.

[murfel]

Oh, double space before comment was intentional. I think I got it from Python.

I wish the IDE reformat action would point this out, too!

Fixed to single space.

[dkhalanskyjb]

No big deal, this is just not the style adopted in our library.

[murfel]

Can we enforce it in automated checks?

[dkhalanskyjb]

I'm open to this (and preventing merging if the style is incorrect), but only if this check doesn't make the CI fail for intermediate commits. It would be annoying to push code just to test something, without ensuring the code is pretty, only to be met with a useless style check complaint. That said, it makes sense to have this check block merging.

[fzhinkin]

Note that instead of making such a check part of the regular CI pipeline, or a pre-commit hook, it could also be implemented as a GH Action, (I'm not 100% sure about this part, but) that'll report all problems right into the PR.

[dkhalanskyjb]

I wonder if we want to remove it afterward. Given that it doesn't seem like uncaught exceptions do anything on Wasm/JS by default, maybe it doesn't matter. Still, avoiding test cross-contamination is worth it so that this doesn't bite us when we forget about this test completely.