Implement DOMNode.getTextContent() according to API

[laeubi]

Fix #1695

[laeubi]

Failures seem unrelated here, ready to be merged.

[angelozerr]

@laeubi it would be very nice if you could add more tests like:

• XML document with just text (ex : foo)
• XML document with several text nodes inside one parent tag (ex : <a><b>f</b>o<b>o</b></a>)
• XML document with several text nodes with several children (ex : <a><b>foo</b><c>bar</c></a>)
• XML document with only comments
• XML document with only processing instructions
• XML document with commants, processing instructions, text nodes
• etc

[laeubi]

Yes more tests would be good, but as this is rather straight forward implementation I do not plan to put too much effort into this especially given that no one complained ever about the old implementation I suspect it is not really important for lemminx users?!?

[angelozerr]

I think it is important for lemminx extensions which can use getTextContent. Today we have some piece of code which uses getTextContent, tests seems working but my fear is that we don't have enough tests to cover your changes.

[laeubi]

The main change is that Elements no don't return null anymore, if anyone relies on this (invalid) behavior that would be the most notable change but one won't catch this with a test of course. I'll try to add some more tests to improve coverage of the different branches but this then only can test what the API of that method requires, not how consumers are using the function.

[angelozerr]

Thanks for adding new tests.

Last changes that I think it could be good:

1. Create a static method assertTextContent in the class to avoid duplicating code (DOMParser....

2. Your tests should look like this

assertTextContent("Hello</a", "Hello")

3. The assertTextContent must have assertNotNull, assertEquals like today

4. And in this assertTextContent I think it could be nice to load xml with xerces to check that lemminx parser have the same result than xerces to be sure that we follow w3c spec

[angelozerr]

@laeubi is there any chance that you do my suggestion in order to merge your PR?

[laeubi]

@laeubi is there any chance that you do my suggestion in order to merge your PR?

Yes I'll hopefully complete that soon.

[angelozerr]

Thanks! I'm sorry to ask you to do that (writting tests are boring) but your PR could have some impacts.

[angelozerr]

@laeubi I am waiting for your update to merge your PR and create the lemminx release

[laeubi]

I tried to implement the method as suggested but at laest locally for me the xerxes parser bails out for CDATA test that it can't find a class that is only available with JDK17... lets see what the CI say.

[laeubi]

Sadly same problem on CI

 Caused by: java.lang.ClassNotFoundException: org.w3c.dom.ElementTraversal
	at java.net.URLClassLoader.findClass(URLClassLoader.java:387)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
	... 39 more

https://stackoverflow.com/questions/17515333/noclassdeffounderror-org-w3c-dom-elementtraversal

says one should use xml-apis-2.10.0.jar

[laeubi]

I now changed this to https://mvnrepository.com/artifact/xml-apis/xml-apis/1.4.01

The xml API seem to be completely wrecked:

• From version number this is the latest: https://mvnrepository.com/artifact/xml-apis/xml-apis/2.0.2 (from 2005)
• But central say it is relocated to https://mvnrepository.com/artifact/xml-apis/xml-apis/1.0.b2 (from 2007)
• From the release date (and usage count) this is the latest https://mvnrepository.com/artifact/xml-apis/xml-apis/1.4.01 (2011)

[laeubi]

@angelozerr seems only the usual build instability now...

[angelozerr]

Thanks @laeubi !