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

fix: make sure assert.throws uses our assert proxy #19

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

jembezmamy
Copy link
Collaborator

This MR adds missing assert.throws compatibility.

In the contrast to other assert methods, assert.throws doesn't use our assert proxy. It reaches for currentTest.assert, which is the original assert object.

The issue can be fixed by overriding the test.assert property with our proxy.

Copy link
Owner

@mrloop mrloop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but could this be done differently, it feels like we'd be able to intercept throws in our Proxy and then handle it correctly there?

@mrloop mrloop added the enhancement New feature or request label Nov 11, 2024
@jembezmamy
Copy link
Collaborator Author

I think we would need to copy whole assert.throws source code and rewrite it in such way, that it uses our assert proxy instead of currentTest.assert.

Another option to investigate is to create test proxy and expose it in assert proxy. And then override test.assert to point back to our proxy. But I'm not sure if it would work (I think I already investigated this option, but I don't remember the details). What's more, config.current still points to a test that has no knowledge about our proxy. So if some custom assertion uses config.current.assert, it won't work with qunit-retry.

So the bottom line is that I don't see any simple alternative solution. :(

Copy link
Owner

@mrloop mrloop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 thanks for the explanation, I agree this looks like the simplest, maybe only solution, feels a bit brittle though :(

@jembezmamy jembezmamy merged commit 2773bd6 into master Nov 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants