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 coverage seems to be very limited #35

Closed
brodybits opened this issue Mar 4, 2020 · 10 comments · Fixed by #59
Closed

test coverage seems to be very limited #35

brodybits opened this issue Mar 4, 2020 · 10 comments · Fixed by #59
Labels
bug Something isn't working help-wanted External contributions welcome
Milestone

Comments

@brodybits
Copy link
Member

npm test continues to succeed if I make some non-sense edits in my work area:

% git diff      
diff --git a/lib/dom-parser.js b/lib/dom-parser.js
index 92937fd..1726488 100644
--- a/lib/dom-parser.js
+++ b/lib/dom-parser.js
@@ -246,6 +246,6 @@ function appendElement (hander,node) {
 var htmlEntity = require('./entities');
 var XMLReader = require('./sax').XMLReader;
 var DOMImplementation = exports.DOMImplementation = require('./dom').DOMImplementation;
-exports.XMLSerializer = require('./dom').XMLSerializer ;
-exports.DOMParser = DOMParser;
+//exports.XMLSerializer = require('./dom').XMLSerializer ;
+//exports.DOMParser = DOMParser;
 //}
diff --git a/lib/dom.js b/lib/dom.js
index 01794c8..5ffcf27 100644
--- a/lib/dom.js
+++ b/lib/dom.js
@@ -1250,7 +1250,7 @@ try{
 }
 
 //if(typeof require == 'function'){
-       exports.Node = Node;
-       exports.DOMImplementation = DOMImplementation;
-       exports.XMLSerializer = XMLSerializer;
+       //exports.Node = Node;
+       //exports.DOMImplementation = DOMImplementation;
+       //exports.XMLSerializer = XMLSerializer;
 //}

I think we need to massively improve the test suite before we can do much more cleanup work.

@brodybits brodybits added bug Something isn't working help-wanted External contributions welcome labels Mar 4, 2020
@brodybits brodybits added this to the 0.4.0 milestone Mar 4, 2020
@brodybits
Copy link
Member Author

I have been using Stryker Mutator on some other projects, highly recommended we consider using it for xmldom.

@brodybits
Copy link
Member Author

I would personally be very open to switching from proof to a well-known test library or framework. I generally like to use Jest in new projects since it just works for both normal JavaScript projects and React Native. I would also not mind adding the new test library before dropping use of the existing proof testlib. This could help unblock testing for PRs like #41.

@karfau
Copy link
Member

karfau commented Jun 19, 2020

I'm very interested in supporting this. any specific tasks/what would be a good first test to have (in a new test framework)?
I have some experience with mocha, tap and jest but none with proof.

@brodybits
Copy link
Member Author

I would start with adding test coverage for the cases in the description.

I think it would be ideal if someone could get Stryker Mutator working with this project.

In terms of test framework, I would probably favor Jest in first place. Jest seems to be used almost always in React Native projects, used in Prettier, and I have come to really like using it. Tap also looks really nice, it looks like it starts with a relatively simple core based on Tape and can be extended with some nicer reporters.

@karfau
Copy link
Member

karfau commented Jun 20, 2020

Wow, I just realize that all the code in the test directory is just some code that dumps stuff to the CLI. No assertions. Update: Found the assertions and am continuing to make them executable.

The only test that run as part of npm run test using proof is that lib/dom-parser.js is truthy...

@karfau
Copy link
Member

karfau commented Jun 20, 2020

I think going for jest will lower the barrier of enrty for may people, which could come in handy in our situation.
And with snapshot testing we could at least assert that it produces the same results that it did before the fork added any changes.
I will give it a try.

@karfau
Copy link
Member

karfau commented Jun 21, 2020

I just found jindw/xmldom#138 saying there is an issue with using xmldom in combination with jest. Maybe it has been fixed already on the jest side, or we will learn something interesting about xmldom 😄

@karfau
Copy link
Member

karfau commented Jun 21, 2020

I digged a bit into the details of testing SAX and existing sax parsers.

It could be worth wile to attempt jindw/xmldom/issues/16 (but there are some isues with it that make it hard).

Which brings me to the (big) question if it's a good idea to maintain both a(nother) SAX parser and all the DOM related things in this project. (I have done the groundwork for comparing different libraries in an efficient manner before, this could be extended/replicated to compare some SAX parsers with our implementation.)

But I think at least having "tons of" fixtures that are checked without writing "tons of" manual tests is a good idea to verify compatibility with DOM specs.

  • either we just find a way to produce XML fixtures and process them
  • or we find a way to generate tests for the API

Any thoughts on this?
(Should we create extra issues for that to communicate intent?)

@brodybits
Copy link
Member Author

Thanks @karfau. I would definitely favor discussing Jest, library comparisons, and DOM specs in separate issues.

There may be some better libraries out there, but I think some projects will continue using xmldom for quite a while (if it ain't broke ...). I got into maintaining the xmldom package since it is used by plist and hence by cordova-ios packages.

@brodybits
Copy link
Member Author

@karfau I just raised #54 & #55 based on your comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help-wanted External contributions welcome
Projects
None yet
2 participants