Skip to content
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

[Feature Request]: Allow more customization of how errors are handled (especially uncaughtrejection). #1736

Open
NullVoxPopuli opened this issue Feb 1, 2024 · 1 comment
Labels
Category: API Component: Assert Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.
Milestone

Comments

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Feb 1, 2024

I've seen the suggestion here: #1419 (comment), but it seems that this is no longer viable due to properties on QUnit being read-only.

This has been the subject of extensive conversation here: emberjs/ember-test-helpers#1128

And while I've figured out a way to test against unhandledrejection by removing QUnit's unhandledrejection event listener, it's a hack at best.
See here: https://github.com/amk221/-ember-unhappy-path-test/pull/1/files#diff-4e063cc3545f3abe7b27e539a9374abb491528a12c0810163bbe8c965b7cc066R9

I understand the philosophy is to "actually handle your rejections", but the reality is that not everyone has control over every error in their projects. Sometimes these could come from libraries / code folks can't really control -- and while in an ideal world, maybe those folks PR to the library's that they're using, but some libraries are complicated, or have a lot of history, and maintainers become afraid of accepting certain changes (this is only one such scenario!)

So, I think it would be good if QUnit exposed some utilities for (on a per-module basis), asserting that exceptions occur in certain scenarios.

Just thought I'd open an issue with some links to more context, to show what people want to do.

@Krinkle
Copy link
Member

Krinkle commented Feb 5, 2024

@NullVoxPopuli I wonder if a new assertion could help here? Something that would tolerate and/or expect a global error.

The implementation could build on the (internal) ignoreGlobalErrors flag that we already set in the HTML Runner's window.onerror handler. This is toggled by assert.throws().

We could expand the use of this flag to also cover uncaught rejections from async functions and promises (by checking it in QUnit.onUncaughtException).

There's a few question raised by this

If we set this from an assertion method, do we want it to merely tolerate it or also to expect it?

Expectation would probably work best with an API that takes a closure. This approach also allows for matching it against an expected error. It could look like this:

QUnit.test('example', function (assert) {
  assert.globalError(function () {
    App.example();
  });

  assert.globalError(function () {
    App.example();
  }, /ReferenceError: boom is not defined/);
});

If we take that approach, one option might be to change the behaviour of assert.throws() to, if the given function didn't throw, to set the actual value to a globally caught error. If we want that, we'll need to think about whether that might cause backward-compatibility issues (i.e. can reasonable test suite that passes today start failing?). At glance, it seems to only widen scenarios under which a given assertion passes, which would be fine.

Tolerance could mean the error is allowed, but not required, i.e. it is ignored. This approach seems more natural if you need to perform assertions on the return value and/or if you want it to span several assertions. This woud mean the error value is not captured.

QUnit.test('example', function (assert) {
  assert.ignoreGlobalErrors(function () {
    assert.propEqual(App.example(), { a: 1 });
  });
});

But... this kind of nesting feels wrong. Mostly because, I've trained myself to detect assertions inside closured as an anti-pattern (given no assertion methods need it today, and when passing functions to other code, assertions performed there are not reliable, I prefer assert.verifySteps). Nesting would also add a burden to developers, as you have to make sure both functions is also async when using await in statements before or at the assertion.

Would setting it on the entire test case be too tolerant? Test cases should be fairly small, so maybe this is fine. Especially considering that today, we don't have a way to either tolerate or expect these, so it feels like a quick way to "deal" with the problem at hand. This should feel very familiar alongside assert.timeout().

By not offering a way to verify the value, and by making it optional, it hints at a usage pattern where you acknowledge tech debt rather than something designed and desirable. I.e. something a bit further away from your control, something potentially non-deterministic, and something that if you cared about it more specifically, you'd work toward moving to be inside your (sync or async) flow control, after which you can perform more specific assertions on it.

And, like with assert.timeout, it can stay behind in your tests after the root cause has been improved and/or fully fixed, without causing a test failure for the inverse.

QUnit.test('example', function (assert) {
  assert.ignoreGlobalErrors():

  assert.propEqual(App.example(), { a: 1 });
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: API Component: Assert Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.
Development

No branches or pull requests

2 participants