Port custom assertions to mostly strict assertions #89

awwright · 2020-07-13T05:39:52Z

I'd like to start chipping away at converting the test suite, I've converted a Vows suite to Mocha before. This one looks pretty straightforward, actually.

I think the first logical step here is to start with the assertions themselves, and use the Node.js core library and only the builtin functions.

In particular, assert.isTrue is a monkey-patch by Vows. I found most all the tests can use assert.strictEqual. There's a few that test if an object is cast to a string, these use assert.equal. There's two more that I think don't meet the DOM standard if you use ===, these use assert.equal and are marked with a FIXME.

karfau · 2020-07-14T09:04:42Z

Thx for the time invested.

From the feedback I got in my first attmept in #51 to do something similar I learned that the owners prefer to keep those tests as they are.
I also wrote the "internal" assert to be able to switch between assert.isTrue and assert.isEqual using the environment variable XMLDOM_ASSERT_STRICT=1. Just removing it again feels strange to me.

I also opened #72 to discuss how to handle testing in the future.
Due to the current status of the code base, my personal suggestion would be to not touch the vows tests and instead start fresh with a new test suite that is better structured.
There has not been a decision yet what test framework to use, but jest and mocha are of course reasonable approaches.

But since I'm only a contributor, I hope the owners will also add their perspective to the mix.

karfau · 2020-07-14T09:30:47Z

.editorconfig

+# http://EditorConfig.org
+root = true
+
+[*.js]
+charset = utf-8
+indent_style = tab
+indent_size = 2
+end_of_line = lf
+insert_final_newline = true


Personally, I believe this is a good idea, but should be added in a separate PR where just this topic can be discussed. See #29

Oops, yes of course

Yes please revert 894522a, thanks.

awwright · 2020-07-14T20:00:53Z

@karfau I don't think having the the tests be dependent on the setting of a variable is a very good idea... I would assume tests cover every situation by default (or at at least as much as the execution environment can do).

But also for the vast majority of the tests, I don't think that makes any sense. If you're testing the length of an array, or the contents of a string, there's no reason to do that non-strict.

Which assertions specifically need the old behavior?

karfau · 2020-07-14T20:23:38Z

@awwright did you read the conversation in #51 (and potentially follow up #59 ) that lead to the creation of those assertions? I'm not really willing to repeat the arguments stated there (, also since they are not mine).

I agree that there are many tests that are not very useful, redundant or hard to maintain.
But's it's what we have right now.

But I would rather invest into a properly structured test suite using modern tooling, that makes the vow's test suite obsolete (by "covering all it covers" and more), instead of "tweaking" the vows test suite and risking that we cover less afterwards. But as I said, it's just my personal preference.
I'm not part of the organization making decisions in this repo.

brodybits · 2020-07-14T20:41:29Z

Hi I am reviewing the changes right now.

I would actually favor converting the existing custom assertions to strict assertions, assuming that it does not affect testing speed too aversely. The testing speed is pretty important for the Stryker mutation testing that is already in place.

I would also not mind if we would port the existing tests from vows to something more universally accepted such as Jest. (I would actually not favor Mocha, but that is personal preference).

I would definitely agree with @karfau that the existing tests are pretty bad, and we may need to replace them all over time, as discussed in #72.

When the custom assert function was introduced in PR #59, my understanding was that this was only a temporary solution to restore the existing test coverage.

I did also take the liberty to update the title.

In short, I do think this is a step in the right direction, even if it is not where we really want to be in the end.

brodybits

In general I think these changes are really awesome. I did leave quite a few comments.

My one major criticism is mixing formatting changes into lines that would otherwise not need to be updated.

I am fine with using "use strict" in the test files, lib should be updated in a separate PR.

brodybits · 2020-07-14T20:16:15Z

test/dom/attr.vows.js

 //not standart
 //    	root.firstChild.setAttributeNode(root.attributes[0]);
-//    	assert(root.attributes.length, 0);
+//    	assert.strictEqual(root.attributes.length, 0);


What should we do with these lines? Any ideas?

Comment them, eventually? Idk what exactly, but if they do get uncommented, this will definitely want to use strictEqual.

brodybits · 2020-07-14T20:21:02Z

.editorconfig

+# http://EditorConfig.org
+root = true
+
+[*.js]
+charset = utf-8
+indent_style = tab
+indent_size = 2
+end_of_line = lf
+insert_final_newline = true


Yes please revert 894522a, thanks.

lib/dom-parser.js

brodybits · 2020-07-14T20:48:30Z

test/dom/clone.vows.js

-    'clone': function () { 
-		var doc1 = new DOMParser().parseFromString("<doc1 attr1='1' attr2='a2'>text1<child>text2</child></doc1>",'text/xml')
+	'clone': function () {
+		var doc1 = new DOMParser().parseFromString("<doc1 attr1='1' attr2='a2'>text1<child>text2</child></doc1>",'text/xml');


I think there is no reason to do formatting change to lines like this at the same time as we convert the assertions. This kind of formatting change makes it harder to see the actual impact of this conversion.

Suggested change

var doc1 = new DOMParser().parseFromString("<doc1 attr1='1' attr2='a2'>text1<child>text2</child></doc1>",'text/xml');

var doc1 = new DOMParser().parseFromString("<doc1 attr1='1' attr2='a2'>text1<child>text2</child></doc1>",'text/xml')

brodybits · 2020-07-14T20:49:45Z

test/dom/clone.vows.js

+		var n =doc1.cloneNode(true);
+		assert(n, doc1s);


I would rather we revert these changes.

Suggested change

var n =doc1.cloneNode(true);

assert(n, doc1s);

var n =doc1.cloneNode(true)

assert(n, doc1s)

brodybits · 2020-07-14T20:58:05Z

test/error/xml-error.vows.js

-	assert(errors.length, 2,"invalid xml attribute(miss qute)")
-	assert(dom+'', '<img attr="1" xmlns="http://www.w3.org/1999/xhtml"/>')
+	assert.strictEqual(errors.length, 2,"invalid xml attribute(miss qute)")
+	assert.strictEqual(dom+'', '<img attr="1" xmlns="http://www.w3.org/1999/xhtml"/>')


I wonder if we should consider this kind of change as well:

Suggested change

assert.strictEqual(dom+'', '<img attr="1" xmlns="http://www.w3.org/1999/xhtml"/>')

assert.strictEqual(dom.toString(), '<img attr="1" xmlns="http://www.w3.org/1999/xhtml"/>')

brodybits · 2020-07-14T20:59:17Z

test/parse/locator.vows.js

@@ -27,7 +29,7 @@ var xml= [
 	var parser = new DOMParser({locator:{}});
    var doc = parser.parseFromString(xml, 'text/xml');
   	var trans = doc.getElementsByTagName('transition')[0];
-   	assert.equal(trans.lineNumber , 10)//,''+trans+trans.lineNumber+'/'+trans.parentNode.previousSibling.previousSibling.lineNumber)
+   	assert.strictEqual(trans.lineNumber, 10);//,''+trans+trans.lineNumber+'/'+trans.parentNode.previousSibling.previousSibling.lineNumber)


Does the commented code indicate another TODO item here?

brodybits · 2020-07-14T21:04:06Z

test/parse/locator.vows.js

@@ -71,6 +73,6 @@ var xml= [
    var doc = parser.parseFromString('<root>\n\t<err</root>', 'text/html');
    var root = doc.documentElement;
    var textNode = root.firstChild;
-	assert.isTrue(/\n@#\[line\:2,col\:2\]/.test(error.join(' ')),'line,col must record:'+JSON.stringify(error));
+	assert(/\n@#\[line\:2,col\:2\]/.test(error.join(' ')),'line,col must record:'+JSON.stringify(error));


A bit hard to read & understand, how about this idea:

Suggested change

assert(/\n@#\[line\:2,col\:2\]/.test(error.join(' ')),'line,col must record:'+JSON.stringify(error));

assert(/\n@#\[line\:2,col\:2\]/.test(error.join(' ')), 'line,col must record: ' + JSON.stringify(error));

or maybe even this:

Suggested change

assert(/\n@#\[line\:2,col\:2\]/.test(error.join(' ')),'line,col must record:'+JSON.stringify(error));

assert(/\n@#\[line\:2,col\:2\]/.test(error.join(' ')), `line,col must record: ${JSON.stringify(error)}`);

or one step further, like what I think Prettier would do:

Suggested change

assert(/\n@#\[line\:2,col\:2\]/.test(error.join(' ')),'line,col must record:'+JSON.stringify(error));

assert(

/\n@#\[line\:2,col\:2\]/.test(error.join(' ')),

`line,col must record: ${JSON.stringify(error)}`

);

brodybits · 2020-07-14T21:05:23Z

test/parse/namespace.vows.js

+       assert.strictEqual(root.namespaceURI, 'http://test.com');
+       assert.strictEqual(root.lookupNamespaceURI(''), 'http://test.com');
+       assert.strictEqual(root.firstChild.namespaceURI, 'http://test.com');
+			 assert.strictEqual(root.firstChild.lookupNamespaceURI(''), 'http://test.com');


fix alignment of this line?

brodybits · 2020-07-14T21:09:18Z

test/parse/test-doc-whitespace.vows.js

+	},
+}).export(module);


I think these should be reverted for now:

Suggested change

},

}).export(module);

}

}).export(module)

awwright · 2020-07-14T21:46:21Z

@brodybits Wrt the rest of the review... It seems to me maybe we should do a PR to normalize the whitespace first, then I can apply these fixes after that. Thoughts?

brodybits · 2020-07-14T21:53:16Z

do a PR to normalize the whitespace first

Yes. I think it would be easier to get the lint in place first. Thanks.

awwright · 2020-09-16T21:17:17Z

I rebased over the updates from #121.

brodybits · 2020-09-17T03:33:50Z

I have updated the title, and I pushed a bunch of updates to give us a green build and make some improvements.

I can think of a couple followup items:

finish removing use of Vows, which is now done in draft PR Completely remove remaining use of Vows #126
use standard Jest expect statements, which would more likely show multiple failed expectations in each test case

karfau · 2020-09-17T20:08:15Z

use standard Jest expect statements, which would more likely show multiple failed expectations in each test case

@awwright Are you still investing time to porting the assertions to strict ones?
I agree that we should rather move them to expect assertions provided by jest in one go.

Or did I misunderstand you @brodybits ?

brodybits · 2020-09-17T20:10:19Z

I could go either way. I would not mind a multi-step approach like this:

port assertions to strict assertions - thanks to @awwright for your efforts!
we use Prettier to reformat, like I proposed in PR apply Prettier formatting --no-semi in test only #123
then we can convert the strict Node.js assertions to expect assertions

awwright · 2020-09-17T20:16:55Z

I was proposing that, after the code is formatted, to first port the assertion calls (as in this PR). That would make changing over the test runner a minimal amount of effort. Then working on fixing tests and adding coverage.

...it seems the test runner has mostly been converted, though.

I favor using assert. It's simple and straightforward.

brodybits · 2020-09-17T20:21:16Z

@awwright we have already switched from Vows to Jest. There are still a few more steps to completely remove Vows, which I have already done in PR #126.

brodybits · 2020-09-17T21:02:39Z

Another thing is that I had spent several hours getting this PR to run with a green build, and find it very unfortunate to see my work thrown away here. (I do have my work saved in my own fork though.)

At this point I am thinking to go ahead with PR #123 with the Prettier reformatting and then convert the custom assertions directly into Jest-style expect assertions.

awwright · 2020-09-17T21:06:42Z

@brodybits I'm sorry, when was that? Was this PR introducing build errors?

brodybits

I think it would be easiest for us to apply the Prettier formatting (PR #123) and then port the custom assertions directly to Jest expect statements. So there should be no need to keep working on this PR.

Thanks to @awwright for the efforts.

awwright · 2020-09-17T21:17:21Z

@brodybits Well, OK, but I'm trying to figure out what changes you're talking about, do you mean a PR? I'm curious which errors you're referring to.

brodybits · 2020-09-17T21:24:17Z

I had pushed the changes to your convert-assertions branch on GitHub. See this: https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork

Unfortunately I don't have much time to explain the errors that I had fixed, due to some other priorities.

In any case, your contributions did help us get the code cleaner. You should be able to see that you have co-authorship credit on some recent changes that we made.

awwright · 2020-09-17T21:58:30Z

I mean, I'm not sure what else to say... GitHub shows my rebases and commits that I pushed, and I see you mentioning this from other PRs, but it doesn't show any commits pushed to my branch.

awwright force-pushed the convert-assertions branch 3 times, most recently from f935823 to b3ee11a Compare July 13, 2020 05:51

karfau reviewed Jul 14, 2020

View reviewed changes

brodybits changed the title ~~Convert Vows assertions to Node.js assertions~~ Convert custom assertions to strict assertions Jul 14, 2020

brodybits reviewed Jul 14, 2020

View reviewed changes

brodybits mentioned this pull request Jul 14, 2020

Lint with tabs & enable strict mode #90

Closed

3 tasks

This was referenced Sep 16, 2020

Port Vows tests to Jest & remove test:vows script #121

Merged

Improve test coverage with future-proof testing strategy #72

Closed

awwright force-pushed the convert-assertions branch from b3ee11a to 3081e08 Compare September 16, 2020 21:14

brodybits mentioned this pull request Sep 17, 2020

enable & enforce strict mode in test modules #124

Merged

brodybits changed the title ~~Convert custom assertions to strict assertions~~ Port custom assertions to mostly strict assertions Sep 17, 2020

brodybits mentioned this pull request Sep 17, 2020

Completely remove remaining use of Vows #126

Merged

2 tasks

awwright force-pushed the convert-assertions branch 5 times, most recently from 8c80c8c to be92b95 Compare September 17, 2020 20:05

Convert Vows assertions to Node.js assertion library

ce6f144

awwright force-pushed the convert-assertions branch from be92b95 to ce6f144 Compare September 17, 2020 20:07

brodybits requested changes Sep 17, 2020

View reviewed changes

awwright closed this Sep 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port custom assertions to mostly strict assertions #89

Port custom assertions to mostly strict assertions #89

awwright commented Jul 13, 2020

karfau commented Jul 14, 2020

karfau Jul 14, 2020

awwright Jul 14, 2020

brodybits Jul 14, 2020

awwright commented Jul 14, 2020

karfau commented Jul 14, 2020

brodybits commented Jul 14, 2020

brodybits left a comment

brodybits Jul 14, 2020

awwright Jul 14, 2020

brodybits Jul 14, 2020

brodybits Jul 14, 2020

brodybits Jul 14, 2020

brodybits Jul 14, 2020

brodybits Jul 14, 2020

brodybits Jul 14, 2020

brodybits Jul 14, 2020

brodybits Jul 14, 2020

awwright commented Jul 14, 2020

brodybits commented Jul 14, 2020

awwright commented Sep 16, 2020

brodybits commented Sep 17, 2020

karfau commented Sep 17, 2020

brodybits commented Sep 17, 2020

awwright commented Sep 17, 2020

brodybits commented Sep 17, 2020

brodybits commented Sep 17, 2020

awwright commented Sep 17, 2020

brodybits left a comment

awwright commented Sep 17, 2020

brodybits commented Sep 17, 2020

awwright commented Sep 17, 2020 •

edited

	var doc1 = new DOMParser().parseFromString("<doc1 attr1='1' attr2='a2'>text1<child>text2</child></doc1>",'text/xml');
	var doc1 = new DOMParser().parseFromString("<doc1 attr1='1' attr2='a2'>text1<child>text2</child></doc1>",'text/xml')

	assert.strictEqual(dom+'', '<img attr="1" xmlns="http://www.w3.org/1999/xhtml"/>')
	assert.strictEqual(dom.toString(), '<img attr="1" xmlns="http://www.w3.org/1999/xhtml"/>')

	assert(/\n@#\[line\:2,col\:2\]/.test(error.join(' ')),'line,col must record:'+JSON.stringify(error));
	assert(/\n@#\[line\:2,col\:2\]/.test(error.join(' ')), 'line,col must record: ' + JSON.stringify(error));

Port custom assertions to mostly strict assertions #89

Port custom assertions to mostly strict assertions #89

Conversation

awwright commented Jul 13, 2020

karfau commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwright commented Jul 14, 2020

karfau commented Jul 14, 2020

brodybits commented Jul 14, 2020

brodybits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwright commented Jul 14, 2020

brodybits commented Jul 14, 2020

awwright commented Sep 16, 2020

brodybits commented Sep 17, 2020

karfau commented Sep 17, 2020

brodybits commented Sep 17, 2020

awwright commented Sep 17, 2020

brodybits commented Sep 17, 2020

brodybits commented Sep 17, 2020

awwright commented Sep 17, 2020

brodybits left a comment

Choose a reason for hiding this comment

awwright commented Sep 17, 2020

brodybits commented Sep 17, 2020

awwright commented Sep 17, 2020 • edited

awwright commented Sep 17, 2020 •

edited