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

Restore vows test "suite", drop useless proof tests #51

Closed
wants to merge 17 commits into from

Conversation

karfau
Copy link
Member

@karfau karfau commented Jun 20, 2020

fixes #35

I found that some part of the test folder are using vows for testing.

  • Main changes in the test files is for fixing imports and removing "traversal code" that vows is able to handle when invoked from the cli. (test/**/.js files that don't have the .vows.js extension might still contain assertions. We need to digest them and decide whether and how to restore them.)
  • To ease filtering I '''renamed''' those files in an extra commit. we should keep the commits when landing so the git history of those files is not lost. (For reviewing it's also much easier to go commit by commit.)

All the vows tests can now be executed via the usual npm test.
Tons of test cases where only calls to console.assert that logged an "Assertion failed: ...", I converted them into assert.[equal|isTrue|greater|...] where possible. When these failed and it made sense, I changed the expectation to the current value.
In case we need to skip a test, the easiest is still to convert it to console.assert.

I also drastically reduced the amount of console.log in tests, now the only thing that's printing in addition to vows is the internal logging of xmldom.

Stryker can be run using npm start for now, I thought it's the most convenient solution for now. See below for the report from my first run.

  • I think we can add jest in a different PR.
  • I found that   is converted to a normal space instead of \u00a0, see comment and commit afterwards that fixes the regression.

Renamed the qualifying `.js` files to `.vows.js` to simplify filtering.
making it pass could be a breaking change
@karfau

This comment has been minimized.

@karfau
Copy link
Member Author

karfau commented Jun 20, 2020

Stryker was super easy to set up! Awesome.

$ npm start
 INFO ConfigReader Using stryker.conf.json
 INFO InputFileResolver Found 4 of 45 file(s) to be mutated.
 INFO InitialTestExecutor Starting initial test run. This may take a while.
 INFO InitialTestExecutor Initial test run succeeded. Ran 1 tests in 2 seconds (net 1070 ms, overhead 2 ms).
 INFO MutatorFacade 1994 Mutant(s) generated
 WARN MutantTestMatcher No coverage result found, even though coverageAnalysis is "all". Assuming that all tests cover each mutant. This might have a big impact on the performance.
 INFO SandboxPool Creating 8 test runners (based on CPU count)
[...]
Ran all tests for this mutant.
Ran 0.97 tests per mutant on average.
---------------|---------|----------|-----------|------------|----------|---------|
File           | % score | # killed | # timeout | # survived | # no cov | # error |
---------------|---------|----------|-----------|------------|----------|---------|
All files      |   55.27 |     1042 |        60 |        892 |        0 |       0 |
 dom-parser.js |   66.18 |      132 |         3 |         69 |        0 |       0 |
 dom.js        |   59.85 |      529 |        30 |        375 |        0 |       0 |
 entities.js   |    2.03 |        5 |         0 |        241 |        0 |       0 |
 sax.js        |   66.07 |      376 |        27 |        207 |        0 |       0 |
---------------|---------|----------|-----------|------------|----------|---------|
 INFO HtmlReporter Your report can be found at: reports/mutation/html/index.html
 INFO Stryker Done in 9 minutes 54 seconds.

@karfau
Copy link
Member Author

karfau commented Jun 20, 2020

I will wait for any reply before going further, I think the PR is already big enough.

@karfau
Copy link
Member Author

karfau commented Jun 21, 2020

To verify that nothing broke since forked, I did the following procedure

// assuming restore-test was checked out and npm i was run once before
git checkout $TAG
git checkout restore-tests -- test
//in test/**/*.vows.js point all imports to the correct location for this version
npx vows test/**/*.vows.js

Result:
v0.1.27 (last version published by original author)

      ✗ text & < 
        »        
        actual expected 
         
        <div xmlns="http://www.w3.org/1999/xhtml"> ©&amp;nbsp&amp;copy</div> 
         // /run/media/karfau/hdd-data/dev/xmldom/node_modules/vows/lib/assert/macros.js:14 
  ✗ Broken » 71 honored ∙ 1 broken (3.567s) 

This is this assertion, so I fixed it by adding back \u00a0.

v0.1.28-not-published (last tag pushed by original author)

      ✗ text & < 
        »        
        actual expected 
         
        <div xmlns="http://www.w3.org/1999/xhtml"> ©&amp;nbsp&amp;copy</div> 
         // /run/media/karfau/hdd-data/dev/xmldom/node_modules/vows/lib/assert/macros.js:14 
  ✗ Broken » 71 honored ∙ 1 broken (3.762s) 

Same assertion as above, so the regression was introduced by him.
Again fixed by adding back normal .

0.3.0 (first version with actual changes, no more v in the tag)

  ✓ OK » 72 honored (3.274s) 

since no code changes where done since that version: DONE 🎉

Copy link
Member

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

In general I think this proposal is really awesome. I would definitely agree to restore vows now and consider whether or not to switch (again) for future consideration.

My biggest criticism is that the change from fb6f516 shows way too much diff on GitHub. I would strongly favor keeping the formatting change in a separate PR and consider using something automatic such as eslint, "Standard JS", or Prettier.

I would also favor moving a few more items out to their own PRs:

  • Assert equal instead of isTrue - do this after we fix the formatting
  • configure Stryker (I would favor using "stryker" rather than "start" in the scripts)
  • fix nbsp regression (maybe raise an issue first)

@brodybits brodybits added the bug Something isn't working label Jun 21, 2020
@karfau
Copy link
Member Author

karfau commented Jun 21, 2020

My biggest criticism is that the change from fb6f516 shows way too much diff on GitHub. I would strongly favor keeping the formatting change in a separate PR and consider using something automatic such as eslint, "Standard JS", or Prettier.

Just to make sure we are talking about the same thing:
When looking at that commit with github I don't see any changes in spaces or tabs or indentation:
image

so I checked the difference in my IDE:
image
So it's line separators... this change was of course unintended. But t's also a bit strange.
The related git setting is set to input since I'm on linux. I'm currently checking the details. It's possible that a mixture of line endings exists in the repo. Assuming I find something, do we generally agree that we should unify the line endings to LF in the repo as soon as possible?

Regarding splitting out into several PRs, that's fine. (I seem to have misunderstood your message in the issue as "stryker" being a requirement to fix/close the issue and assuming they should go into one PR.)

@brodybits
Copy link
Member

So it's line separators...

yup

It's possible that a mixture of line endings exists in the repo.

I would not be surprised.

I generally use command-line tools together with gitx, didn't think of anything going on with the IDE.

I would actually favor that we restore the test coverage before dealing with the LF endings or any other formatting items.

In terms of Stryker, I could go either way at this point if the total changes do not prove excessive. I would still really like to get the nbsp fix into its own PR (and maybe an issue).

Another side point is that the name of test/parse/test-doc-witespace.js has a misspelling, it should probably be changed to: test/parse/test-doc-whitespace.vows.js.

@karfau karfau marked this pull request as draft June 22, 2020 05:37
@karfau
Copy link
Member Author

karfau commented Jun 22, 2020

Just thinking out loud here (and for referencing it later), this is my current plan:

@karfau
Copy link
Member Author

karfau commented Jul 1, 2020

FYI: The checklist is complete, I will still keep the branch since there are some links to it by now and maybe also some other changes that we could pick from it. But nothing urgent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test coverage seems to be very limited
2 participants