test: Add jest testsuite with `xmltest` cases #112

karfau · 2020-08-21T09:55:07Z

The only thing the current test suite does is to run ton's of cases and take a snapshot of the output and errors, so that hopefully a test will fail if we make a breaking change.
(It doesn't cover breaking API changes of course, but sets the basis since it adds jest and any added test will be part of CI testing.)

Added devDependencies:

https://github.com/karfau/xmltest and it's peerDeps
- https://npms.io/search?q=yauzl
- https://npms.io/search?q=get-stream

New helper Module get-test-parser.js:
To simplify the very common use case of testing DOMParser results.

brodybits · 2020-08-21T19:01:48Z

Thanks @karfau, I may need a few days to review this. I definitely like the runex tool that you are using. Some quick comments:

I like to keep binaries out of git whenever possible. I am wondering if the xmltest tool could be published as a separate utility package?
I would like to first add tests to cover functionality not tested then do a gradual transition to bring the existing tests out of their existing structure. I think you are right that the tests should be mostly rewritten in the end.

karfau · 2020-08-21T19:55:09Z

I like to keep binaries out of git whenever possible. I am wondering if the xmltest tool could be published as a separate utility package?

Just to clarify: The xmltest "tool" is just a zip file containing xml files as test cases. The only thing that is binary is the file format (zip). The reason for that is the license attached to those files (in the readme.html inside the zip file):

Copyright (C) 1998 James Clark. All rights reserved. Permission is
granted to copy and modify this collection in any way for internal use
within a company or organization. Permission is granted to
redistribute the file xmltest.zip containing this
collection to third parties provided that no modifications of any kind
are made to this file. Note that permission to distribute the
collection in any other form is not granted.

I understand your concern, that is why I initially tried to get a license to use it in an unpacked way, but the person never replied to me. (I cc'ed you in the mail, you might remember.)

We could of course just fetch the file from it's original sources, or we could upload it to a different location and fetch it from there. Or package it into an npm package (e.g. [@xmldom/]xmltest, which could also contain a nice way to unzip it for testing. Is that what you have in mind?

We can of course make that change, but from my perspective this just feels more complex. And since this 100k file will most likely never change I don't think it's a huge problem.

If you this it is required for landing the PR I can of course invest time into it. (Would need help if you want it to be published by the xmldom org.)

karfau · 2020-08-21T20:10:35Z

I would like to first add tests to cover functionality not tested then do a gradual transition to bring the existing tests out of their existing structure. I think you are right that the tests should be mostly rewritten in the end.

This test adds test for things that are not covered yet. I will hurry up making the stryker config work, it will prove that point. The cases are provided by the quite elaborate and well structured testcase suite inside xmltest.zip.
Checking out the branch and running npm run pretest:jest unpacks it into test/xmltest for further exploration. The contained readme.html is a good starting point.

If you want I can of course also split the RP into:

introduce jest
add the xmltest test suites

But as I stated above this is my perspective:
The main goal of this approach was a fast way to make sure we are aware when changes introduce breaking changes. I'm quite certain we are able to rely on this test suite much better then on the old one.

Once this is provided

transitioning (including restructuring if we want) tests to jest
fixing bugs
adding features
can be worked on in parallel with much more confidence.

I initially created #72 to discuss this topic, but we didn't manage to continue the conversation there. So this PR is a more concrete version of what I had/have in mind.

This being said, please take your time to review and/or let me know what you think about it.

brodybits · 2020-08-21T20:24:38Z

I am thinking we should keep the xmltest cases in a special subdirectory, and dynamically download xmltest.zip as needed. It may make sense to consider this a form of integration testing. I did not realize before that it has its own license, really do not want to include it in this git repository.

I would probably favor introducing Jest in its own PR, probably with a very limited number of simple, clean test(s) to prove it working.

Some more high-level comments are coming into #72. I may need some more time to review this PR more closely. Thanks.

karfau · 2020-08-21T21:34:23Z

FYI:

Just completed running npm run stryker on this branch (performance is not ideal), it has a score of 99.40%. report.zip
I will prepare another PR that just introduces jest and adds a small amount of tests that might be able to "fill the wholes" stryker points to in the above report.
I'm working on a way to remove the zip file from this branch, but it may take a while.
I will only update the branch if I'm able to make that work, or in case the other jest-only PR lands, so it can be reviewed in peace. (Or maybe I will create another one for the second iteration and close this PR when I'm able to show it? We will see.)

brodybits

I left a few nits for test/createParser.js.

I am also wondering if we should raise bug reports for the errors we encounter between xmldom and xmltest?

P.S. Thanks for your efforts with Jest and the zip file. I am now wondering if there could be any way to synchronously process contents from a zip file?

brodybits · 2020-08-30T19:18:11Z

test/createParser.js

+function createParser ({errorHandler, errors = {}, locator = {}} = {}) {
+  errorHandler = errorHandler || ((key, msg) => {
+    if (!errors[key]) errors[key] = []
+    errors[key].push(msg)


minor nit: I think newly added code should avoid overwriting function parameters unless absolutely necessary, which I think is not the case here.

nit 2: I find the file name createParser.js a little too generic. If it is intended to be used by other tests in the future, I would favor calling it something like createTestParser.js. If it is only expected to be used for xmltest, I would favor reflecting that in the module name.

another minor nit: I think all other modules in lib and test are in kebab case, which I would probably favor.

In this case it's actually nessesary to pass those parameters, tsince this way the "errors, warnings and fatals" are part of the snapshot assertion.Should one of them go missing or change the test would break.
The error handlers are part of the public API and the code to handle them the same way in multiple tests is quite verbose, to I decided to extract them.
Since it's still work in progress, I really don't bother to much with names.
I'm also not sure if we should use it outside of the xmltest "suite", so I'm fine with whatever name you prefer.

kebab case

You are right! I really didn't realize that "convention" existed consistently in the test folder. Most modules in this repo, especially in the test folder are not really helpful with their naming, and it felt natural to name a module after the only function it exports.
But I don't have any concerns with renaming it.

Thinking about it a bit longer: maybe I just didn't get your point about "overriding".
If my reply was not satisfying, please let me know and elaborate your concerns.

The renaming took place, please check if this is what you had in mind.

brodybits · 2020-08-30T20:39:30Z

I temporarily block edits for this PR since I have quite some changes locally already.

Makes sense. I am assuming (and hoping) you will also rebase (or merge) to resolve the conflicts:)

"ugly" way: extract to disk (e.g. in postinstall script and use fs.readSync in the tests.

I would be fine if we do this into some kind of a temp directory such as .tmp or .temp and add it to .gitignore. I think this should be able to unblock progress without any issue with the license.

That said, it would definitely be nice if we can find a better solution.

I would love it if we could get this fixed in Jest somehow. Probably a long shot.

brodybits · 2020-08-30T20:42:57Z

Hey how about this one: https://nodejs.org/api/zlib.html#zlib_zlib_unzipsync_buffer_options

karfau · 2020-08-31T06:51:12Z

I had a nice idea over night and it works quite well. So I don't need to look into other options.

I think this PR will be ready in some days.

karfau · 2020-09-03T20:19:45Z

I am also wondering if we should raise bug reports for the errors we encounter between xmldom and xmltest?

I'm very satisfied with the current status, so I can go ahead and file that issue, and can refer to it in the comment.

brodybits

I left some questions and comments. In general:

test/xmltest/__snapshots__/not-wf.test.js.snap seems to be a binary file
When I did npm install on my mac, it seems to remove some entries from package-lock.json. I will probably regenerate package-lock.json anyway before release, but was wondering about this.

babel.config.js

jest.config.js

brodybits · 2020-09-04T04:07:34Z

stryker.conf.json

-    "command": "npm run test:unit"
-  }
+    "command": "jest"
+  },
+  "files": [
+    "test/**/*.test.js", "test/**/*.xml", "test/**/*.html", "test/createParser.js", "lib/*.js", "package.json", "jest.config.js"
+  ]


I think these changes are not needed. We should run Stryker with the entire test suite.

This was here for two reasons:

improve performance of copying into .stryker-tmp folders (I don't think it's needed anymore)

My expectation is that the xmltest suite covers everything (and more) that the vows testsuite does. I'll double check. If the current score of 99.7% is increased by also running the vows tests as part of stryker, I would of course include them.

That being said, using npm to run jest and vows will be a major performance drain for running stryker: only jest takes 2 seconds on my machine, npm run test:unit takes 8 seconds.

The result is outside of how I understood stryker so far:

stryker command Mutation score # Killed # Survived # Timeout # No coverage # Ignored # Runtime errors # Compile errors Total detected Total undetected Total mutants

jest 99.70% 1958 6 35 (timeoutMS:15s) 0 0 0 0 1993 6 1999

npm run test:unit or npm run test:vows && npm run test:jest 64.73% 1245 705 409 (timeoutMS:30s) 0 0 0 0 1294 705 1999

The higher amounts of timeout is most likely related to tests taking to long (I have see this number go up drastically when the timeout it to low.
But why would more tests result in less coverage?

I will try to see if that is an issue with npm-run-all ...

Update: no change, see updated table above.
Filed stryker-mutator/stryker-js#2459 to verify my assumptions. If you understand that, please help me.

stryker.jest.conf.json

test/configure-test-parser.js

test/xmltest/not-wf.test.js

test/xmltest/valid.test.js

karfau · 2020-09-04T05:21:39Z

Sorry for wasting some of your time since you looked at it so fast. I found out this morning that it was not totally ready.

But I can already reply to some of your comments:

test/xmltest/snapshots/not-wf.test.js.snap seems to be a binary file

No, It's a snapshot file from jest. I'll tell git(hub) to display their diff.
Update: adding the .gitattributes file only helped for one file but not the other. But clicking on "view file" shows a perfectly plain text file.

package-lock.json

I'll check.

I might need to force push again to get rid of some stuff, so I'll prevent edits for now. => Done

The only thing the current test suite does is to run ton's of cases and take a snapshot of the output and errors, so that hopefully a test will fail if we make a breaking change. (It doesn't cover breaking API changes of course, but sets the basis since it adds jest and any added test will be part of CI testing.) Added devDependencies: - https://npms.io/search?q=jest - https://github.com/karfau/xmltest and it's peerDeps - https://npms.io/search?q=yauzl - https://npms.io/search?q=get-stream New helper Module `configure-test-parser.js`: To simplify the very common use case of testing DOMParser results

(Couldn't make the pure jestRunner solution work)

brodybits · 2020-09-04T19:22:07Z

Thanks. I may need a few days to finish reviewing.

Unfortunately git and GitHub seem to continue to treat test/xmltest/__snapshots__/not-wf.test.js.snap like a binary file. I think .gitattributes is not right. I tried fixing it like this, but it still treats the snapshot like a binary file:

# jest snapshots can and should show in diff, since they are not binary
*.test.js.snap text

When I tried running test/xmltest/not-wf.test.js with this change:

--- a/test/xmltest/not-wf.test.js
+++ b/test/xmltest/not-wf.test.js
@@ -24,7 +24,7 @@ describe('xmltest/not-wellformed', () => {
                                // for 050.xml the result is undefined so `.toString()` needs to be careful
                                const actual = parser.parseFromString(input)
 
-                               expect({actual: actual ? actual.toString() : actual, errors}).toMatchSnapshot()
+                               expect({input, actual: actual ? actual.toString() : actual, errors}).toMatchSnapshot()
                        })
                })
        })

then I could see from the snapshot some inputs that result in some non-text outputs:

&#RE;
&#X58;

and an input that results in an error:

&e;

I am thinking that we should replace the few troublesome non-text output sequences and be able to get rid of .gitattributes.

test/xmltest/not-wf.test.js

test/xmltest/valid.test.js

This comment has been minimized.

Sign in to view

brodybits mentioned this pull request Aug 21, 2020

Improve test coverage with future-proof testing strategy #72

Closed

7 tasks

karfau mentioned this pull request Aug 26, 2020

test: Add jest and first tests #114

Merged

This comment has been minimized.

Sign in to view

brodybits reviewed Aug 30, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

karfau force-pushed the jest-xmltest branch 2 times, most recently from 70d9ef3 to b1d3c6b Compare September 3, 2020 20:14

karfau marked this pull request as ready for review September 3, 2020 20:19

brodybits reviewed Sep 4, 2020

View reviewed changes

karfau marked this pull request as draft September 4, 2020 04:58

karfau added 3 commits September 4, 2020 07:52

style: Tell git(hub) to show diff for snapshot files

ad88291

test: Only run jest for stryker but using commandRunner

5124cbf

(Couldn't make the pure jestRunner solution work)

karfau force-pushed the jest-xmltest branch from b1d3c6b to 5124cbf Compare September 4, 2020 05:53

karfau marked this pull request as ready for review September 4, 2020 11:25

brodybits reviewed Sep 4, 2020

View reviewed changes

test/xmltest/not-wf.test.js Outdated Show resolved Hide resolved

brodybits reviewed Sep 4, 2020

View reviewed changes

test/xmltest/valid.test.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add jest testsuite with `xmltest` cases #112

test: Add jest testsuite with `xmltest` cases #112

karfau commented Aug 21, 2020 •

edited

This comment has been minimized.

brodybits commented Aug 21, 2020

karfau commented Aug 21, 2020 •

edited

karfau commented Aug 21, 2020

brodybits commented Aug 21, 2020 •

edited

karfau commented Aug 21, 2020 •

edited

This comment has been minimized.

brodybits left a comment •

edited

brodybits Aug 30, 2020

karfau Aug 30, 2020

karfau Sep 3, 2020

This comment has been minimized.

brodybits commented Aug 30, 2020

brodybits commented Aug 30, 2020

karfau commented Aug 31, 2020 •

edited

karfau commented Sep 3, 2020

brodybits left a comment

brodybits Sep 4, 2020

karfau Sep 4, 2020 •

edited

karfau Sep 4, 2020 •

edited

karfau commented Sep 4, 2020 •

edited

brodybits commented Sep 4, 2020

This comment has been minimized.

stryker command	Mutation score	# Killed	# Survived	# Timeout	# No coverage	# Ignored	# Runtime errors	# Compile errors	Total detected	Total undetected	Total mutants
`jest`	99.70%	1958	6	35 (timeoutMS:15s)	0	0	0	0	1993	6	1999
`npm run test:unit` or `npm run test:vows && npm run test:jest`	64.73%	1245	705	409 (timeoutMS:30s)	0	0	0	0	1294	705	1999

test: Add jest testsuite with xmltest cases #112

test: Add jest testsuite with xmltest cases #112

Conversation

karfau commented Aug 21, 2020 • edited

This comment has been minimized.

brodybits commented Aug 21, 2020

karfau commented Aug 21, 2020 • edited

karfau commented Aug 21, 2020

brodybits commented Aug 21, 2020 • edited

karfau commented Aug 21, 2020 • edited

This comment has been minimized.

brodybits left a comment • edited

Choose a reason for hiding this comment

brodybits Aug 30, 2020

Choose a reason for hiding this comment

karfau Aug 30, 2020

Choose a reason for hiding this comment

karfau Sep 3, 2020

Choose a reason for hiding this comment

This comment has been minimized.

brodybits commented Aug 30, 2020

brodybits commented Aug 30, 2020

karfau commented Aug 31, 2020 • edited

karfau commented Sep 3, 2020

brodybits left a comment

Choose a reason for hiding this comment

brodybits Sep 4, 2020

Choose a reason for hiding this comment

karfau Sep 4, 2020 • edited

Choose a reason for hiding this comment

karfau Sep 4, 2020 • edited

Choose a reason for hiding this comment

karfau commented Sep 4, 2020 • edited

brodybits commented Sep 4, 2020

This comment has been minimized.

test: Add jest testsuite with `xmltest` cases #112

test: Add jest testsuite with `xmltest` cases #112

karfau commented Aug 21, 2020 •

edited

karfau commented Aug 21, 2020 •

edited

brodybits commented Aug 21, 2020 •

edited

karfau commented Aug 21, 2020 •

edited

brodybits left a comment •

edited

karfau commented Aug 31, 2020 •

edited

karfau Sep 4, 2020 •

edited

karfau Sep 4, 2020 •

edited

karfau commented Sep 4, 2020 •

edited