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 ua asserts issue without tap #159

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

dmanto
Copy link
Contributor

@dmanto dmanto commented Nov 11, 2024

Summary

This PR addresses an issue with TestUserAgent where several assertion methods, such as .jsonIs(), did not work when using the native node:assert/strict module, specifically due to missing assertion methods like assert.same().

Fix

  • Implemented missing assertion methods to ensure compatibility with node:assert/strict, such as replacing assert.same() with assert.deepStrictEqual() for deep equality checks.
  • Updated the code to support both tap and node:assert/strict modes in the TestUserAgent to provide flexibility in testing.

Changes

  • Mapped _assert methods to TestUserAgent to support the required assertions:
    • assert.equalassert.strictEqual
    • assert.notassert.notStrictEqual
    • assert.matchassert.match
    • assert.notMatchassert.doesNotMatch
    • assert.okassert.ok
    • assert.notOk → custom implementation for negated assert.ok
    • assert.sameassert.deepStrictEqual
  • Updated tests to validate assertion compatibility in both tap and node:assert/strict modes.

Caveat

This fix maps node:assert/strict methods to their tap counterparts. This could give the impression that we are prioritizing the tap API over the native node:assert API.

If preferred, we can consider making node:assert/strict the primary module for assertions in TestUserAgent and refactor the code accordingly.

@kraih
Copy link
Member

kraih commented Nov 11, 2024

Yes, node:assert should be the priority.

@dmanto
Copy link
Contributor Author

dmanto commented Nov 14, 2024

Hi, just checking if you had any further feedback on this approach after the refactor prioritizing node:assert. Let me know if there's anything else you'd like me to address

@kraih kraih merged commit cbf4836 into mojolicious:main Nov 18, 2024
10 checks passed
@kraih
Copy link
Member

kraih commented Nov 18, 2024

Thanks, that seems like a reasonable solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants