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

refactor(tests): convert to TypeScript #508

Merged
merged 1 commit into from Dec 26, 2019
Merged

Conversation

mightyiam
Copy link
Contributor

@mightyiam mightyiam commented Dec 19, 2019

Closes #293.

Fixes #509.

Copy link
Contributor Author

@mightyiam mightyiam left a comment

Choose a reason for hiding this comment

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

OK cool.

.ecrc Outdated
Comment on lines 1 to 42
{
"ignoreDefaults": true,
"exclude": [
"^coverage/",
"^node_modules/",
"^es/",
"^helpers/",
"^modules/",
"^test/",
"^h.d.ts",
"^h.js",
"^h.js.map",
"^hooks.d.ts",
"^hooks.js",
"^hooks.js.map",
"^htmldomapi.d.ts",
"^htmldomapi.js",
"^htmldomapi.js.map",
"^is.d.ts",
"^is.js",
"^is.js.map",
"^jsx.d.ts",
"^jsx-global.d.ts",
"^jsx.js",
"^jsx.js.map",
"^snabbdom.bundle.d.ts",
"^snabbdom.bundle.js",
"^snabbdom.bundle.js.map",
"^snabbdom.d.ts",
"^snabbdom.js",
"^snabbdom.js.map",
"^thunk.d.ts",
"^thunk.js",
"^thunk.js.map",
"^tovnode.d.ts",
"^tovnode.js",
"^tovnode.js.map",
"^vnode.d.ts",
"^vnode.js",
"^vnode.js.map"
]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is just #506.

@@ -33,6 +33,7 @@ node_modules
/es/modules/style.d.ts
/es/modules/style.js
/es/modules/style.js.map
/es/test/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have new build artifacts. The tests. They should be ignored along with the rest of the build artifacts. This is from the ES modules build.

@@ -92,6 +93,7 @@ node_modules
/modules/style.d.ts
/modules/style.js
/modules/style.js.map
/test/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above. This is from the CommonJS build.

@@ -19,7 +19,7 @@ module.exports = function(config) {
basePath: '.',
frameworks: ['mocha', 'karma-typescript'],
// list of files / patterns to load in the browser
files: [{pattern: 'src/**/*.ts'}, {pattern: 'test/**/*'}],
files: [{pattern: 'src/**/*.ts'}],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests are moved to src/test so no two patterns. Just one.

'src/**/*.ts': ['karma-typescript'],
'test/**/*.{js,ts,tsx}': ['karma-typescript'],
'src/**/*.{ts,tsx}': ['karma-typescript']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only one preprocessor definition, because everything is under src. And there's no .js file anywhere in src.


var snabbdom = require('../snabbdom');
fakeRaf.use();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using require calls, with whatever karma uses as a module system, this .use() call occurs before the style module is evaluated. But converting this to TypeScript and static imports, the only way to have the .use() call occur before the import, seems to be using dynamic import() call. If we had top-level await then it would have been reasonable. But TypeScript doesn't support that yet.

So I removed this sweet little tool by @paldepind and replaced it with actual calls to the real requestAnimationFrame. See the test below.

Comment on lines -70 to 73
var vnode1 = h('div', {style: {'--myVar': 1}});
var vnode2 = h('div', {style: {'--myVar': 2}});
var vnode3 = h('div', {style: {'--myVar': 3}});
var vnode1 = h('div', {style: {'--myVar': 1 as any}});
var vnode2 = h('div', {style: {'--myVar': 2 as any}});
var vnode3 = h('div', {style: {'--myVar': 3 as any}});
elm = patch(vnode0, vnode1).elm;
assert.equal(elm.style.getPropertyValue('--myVar'), 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder why these even use numbers instead of strings.

What I find interesting is that they come out as numbers on the other side, as well.

Comment on lines -110 to +118
fakeRaf.step();
fakeRaf.step();
assert.equal(elm.style.fontSize, '16px');
elm = patch(vnode1, vnode2).elm;
assert.equal(elm.style.fontSize, '18px');
fakeRaf.step();
fakeRaf.step();
assert.equal(elm.style.fontSize, '20px');
requestAnimationFrame(() => {
requestAnimationFrame(() => {
assert.equal(elm.style.fontSize, '16px');
elm = patch(vnode1, vnode2).elm;
assert.equal(elm.style.fontSize, '18px');
requestAnimationFrame(() => {
requestAnimationFrame(() => {
assert.equal(elm.style.fontSize, '20px');
done()
})
})
})
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I little bit more indented, but works. One thing I don't understand is why it takes two calls to read the change. Anyone?

@@ -9,6 +9,7 @@
"noUnusedLocals": true,
"jsx": "react",
"jsxFactory": "jsx",
"esModuleInterop": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the imports required this. I don't even remember. Whatever. Should be on anyway.

Comment on lines +29 to +37
"src/test/attachto.ts",
"src/test/attributes.ts",
"src/test/core.ts",
"src/test/dataset.ts",
"src/test/eventlisteners.ts",
"src/test/htmldomapi.ts",
"src/test/jsx.tsx",
"src/test/style.ts",
"src/test/thunk.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, TypeScript, please compile these, as well.

@mightyiam
Copy link
Contributor Author

This should open the door to solving some type issues we have with confidence, because the code changes would be tested in the tests. Because the tests would be TypeScript and not just JavaScript. Forgive my English. It's 4:29 am.

@kuraga
Copy link

kuraga commented Dec 19, 2019

Just as a side note: T-man: Super test manager for JavaScript. (Lightweight.)

@mightyiam mightyiam merged commit b77cb51 into master Dec 26, 2019
@mightyiam mightyiam deleted the typescript-tests branch December 26, 2019 12:39
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.

Karma coverage seems to include tests under Windows Convert tests to TypeScript
2 participants