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

test: migrate from tap to node:test and c8 #843

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

R-Campbell
Copy link
Contributor

A few decisions while converting tests:

  • t.same and t.match were converted to t.assert.deepEqual.
  • t.strictSame was converted to t.assert.deepStrictEqual.
  • t.notOk was converted to t.assert.equal comparing the coerced value (using !!) to false. We could have done t.assert.ok(${value} === false) but using equal with false felt more in the spirit of checking that for things that are not ok.
  • t.pass was converted to t.assert.ok(true, ${originalMessage})
  • Snapshots were removed and copied into the appropriate tests.

Checklist

Copy link
Member

@dancastillo dancastillo left a comment

Choose a reason for hiding this comment

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

Can you update t.assert.equal to t.assert.strictEqual and t.assert.deepEqual to t.assert.deepStrictEqual

.gitignore Outdated
Comment on lines 151 to 153
#tap files
.tap/

Copy link
Member

Choose a reason for hiding this comment

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

Might be worth keeping this around for a bit just in case for people who ran tests

@Fdawgs Fdawgs changed the title fix: migrate from tap to node:test and c8 test: migrate from tap to node:test and c8 Dec 13, 2024
@R-Campbell
Copy link
Contributor Author

I made that update but ran into an issue with the Symbol that json-schema-resolver adds to the schema (specifically json-schema-resolver.ignore) and couldn't find a great way to reference that in the testing code. Instead I used JSON.parse(JSON.stringify(...)) to strip the Symbols from the schema objects where needed (see here for an example). I don't know if that's the best solution, but it works.

I also found some places where null needed to be updated to undefined (see here for an example) as well as t.fail calls that I missed and changed to t.assert.fail.

Copy link
Member

@dancastillo dancastillo left a comment

Choose a reason for hiding this comment

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

Lets removed the !! to do a strict assert of undefined like this t.assert.strictEqual(!!value, undefined)t.assert.strictEqual(value, undefined)` this will help explain behavior IMHO

test/mode/static.js Outdated Show resolved Hide resolved
t.ok(definedPath)
t.match(definedPath.responses[200].content, {
t.assert.ok(definedPath)
t.assert.deepEqual(JSON.parse(JSON.stringify(definedPath.responses[200].content)), {
Copy link
Member

Choose a reason for hiding this comment

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

this works without JSON.parse(JSON.stringify(...)

t.ok(definedPath)
t.match(definedPath.requestBody.content, {
t.assert.ok(definedPath)
t.assert.deepEqual(JSON.parse(JSON.stringify(definedPath.requestBody.content)), {
Copy link
Member

Choose a reason for hiding this comment

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

this works without JSON.parse(JSON.stringify(...)

@dancastillo
Copy link
Member

I made that update but ran into an issue with the Symbol that json-schema-resolver adds to the schema (specifically json-schema-resolver.ignore) and couldn't find a great way to reference that in the testing code. Instead I used JSON.parse(JSON.stringify(...)) to strip the Symbols from the schema objects where needed (see here for an example). I don't know if that's the best solution, but it works.

I also found some places where null needed to be updated to undefined (see here for an example) as well as t.fail calls that I missed and changed to t.assert.fail.

@Eomm wdyt here - JSON.parse(JSON.stringify(...)) does remove the symbol from the object. Wondering if there is a preferred way to handle this?

R-Campbell and others added 5 commits December 16, 2024 22:29
Co-authored-by: Dan Castillo <dan.castillo@jasper.ai>
Signed-off-by: Robert Campbell <R-Campbell@users.noreply.github.com>
Signed-off-by: Robert Campbell <R-Campbell@users.noreply.github.com>
change from coerced value comparison against false to actual value comparison against undefined where necessary
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