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.

Sign up for GitHub

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

Jump to bottom

Update tsconfig.json, use node:test #21

Merged
merged 8 commits into from
Feb 6, 2023
Merged

Update tsconfig.json, use node:test #21

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3204843
upgrade hast util raw to version 8, use node:test and node:assert
ChristianMurphy Jan 17, 2023
f78e3af
ci: upgrade node version to lts/fermium
ChristianMurphy Jan 17, 2023
ceba3a6
ci: upgrade node to lts gallium
ChristianMurphy Jan 17, 2023
b657c8d
test: use default exports in test suite
ChristianMurphy Jan 17, 2023
535b272
type: use node16 module resolution
ChristianMurphy Jan 17, 2023
d306d39
types: include node.js types in devDependencies
ChristianMurphy Jan 17, 2023
038f8be
revert hast-util-raw update, remove @types/tape
ChristianMurphy Jan 19, 2023
e30f01d
test: make test async
ChristianMurphy Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ jobs:
strategy:
matrix:
node:
- lts/erbium
- lts/gallium
- node
3 changes: 1 addition & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"unified": "^10.0.0"
},
"devDependencies": {
ChristianMurphy marked this conversation as resolved.
Show resolved Hide resolved
"@types/tape": "^4.0.0",
"@types/node": "^18.0.0",
"c8": "^7.0.0",
"prettier": "^2.0.0",
"rehype-stringify": "^9.0.0",
Expand All @@ -44,7 +44,6 @@
"remark-preset-wooorm": "^9.0.0",
"remark-rehype": "^10.0.0",
"rimraf": "^3.0.0",
"tape": "^5.0.0",
"type-coverage": "^2.0.0",
"typescript": "^4.0.0",
"unist-util-visit": "^4.0.0",
Expand Down
13 changes: 6 additions & 7 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,16 @@
* @typedef {import('hast').Root} Root
*/

import test from 'tape'
import test from 'node:test'
import assert from 'node:assert/strict'
import {unified} from 'unified'
import {visit} from 'unist-util-visit'
import remarkParse from 'remark-parse'
import remarkRehype from 'remark-rehype'
import rehypeStringify from 'rehype-stringify'
import rehypeRaw from './index.js'

test('rehypeRaw', (t) => {
t.plan(2)

test('rehypeRaw', () => {
unified()
.use(remarkParse)
.use(remarkRehype, {allowDangerousHtml: true})
Expand All @@ -21,7 +20,7 @@ test('rehypeRaw', (t) => {
/** @type {import('unified').Plugin<Array<void>, Root>} */
() => (root) => {
visit(root, 'raw', () => {
t.fail('should not include `raw` in tree after `rehype-raw`')
assert.fail('should not include `raw` in tree after `rehype-raw`')
})
}
)
Expand All @@ -35,9 +34,9 @@ test('rehypeRaw', (t) => {
'</div>'
].join('\n'),
(error, file) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is the execution order now:

  1. the test runs
  2. The unified processor is started
  3. The test succeeds
  4. The unified processor calls a callback
  5. Assertions are made

Previously tape could use the t context to determine how many assertions were expected, and wait for those. This is no longer the case.

IMO the cleanest solution is to either:

  • use the unified promise API, and async / await syntax for this test.
  • use the unified sync API.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it has been reordered in that way.
I tried adding assert.fail() inside the callback to see if it would wait long enough for it to fail.
It does fail.

I'm open to making it async if that would be more clear for folks.
But I don't think the change is required for the tests to function.

Copy link
Member

Choose a reason for hiding this comment

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

I think Remco here is correct, due to plan. That makes tape run async and wait until there are two assertions.

I think what’s happening now is just uncaught exceptions (because assert throws errors, tape doesn’t). While it “works” (because uncaught exceptions still fail the process), I don’t think that’s a good idea?

So I believe your assert.fail causes an uncaught exception?

I like using async/await now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in fc02c20

t.ifErr(error, 'should not fail')
assert.ifError(error)

t.equal(
assert.equal(
String(file),
[
'<div class="note">',
Expand Down
9 changes: 3 additions & 6 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
{
"include": ["*.js"],
"compilerOptions": {
"target": "ES2020",
"lib": ["ES2020"],
"module": "ES2020",
"moduleResolution": "node",
"allowJs": true,
"target": "es2021",
"lib": ["es2021"],
"module": "node16",
"checkJs": true,
"declaration": true,
"emitDeclarationOnly": true,
"allowSyntheticDefaultImports": true,
"skipLibCheck": true,
ChristianMurphy marked this conversation as resolved.
Show resolved Hide resolved
"strict": true
}
Expand Down