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

WIP: reduce use of xalan #721

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from
Draft

Conversation

pjfanning
Copy link
Contributor

@pjfanning pjfanning commented Jul 23, 2022

Description

See:

This is a POC - if the changes look like a valid way forward, I can put more effort in.
Some tests are failing and will be investigated
Probably need some new tests too
The ant build.xml needs changes - is Ant still used?

Motivation and Context

How Has This Been Tested?

unit tests

Screenshots (if appropriate):

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Probably not a huge deal. Main worry is that some XPath expressions may not behave exactly the same as before. Another issue is that PropertiesBasedPrefixResolver and PropertiesBasedPrefixResolverForXpath2 are public classes that extend a xalan interface (PrefixResolver) - this POC removes the use of that xalan interface.

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.

@pjfanning pjfanning marked this pull request as draft July 23, 2022 11:52
@FSchumacher
Copy link
Contributor

Ant is still used to generate our docs. I think all xalan related stuff can be deleted safely.

@@ -276,4 +278,19 @@ public void testPutValuesForXPathInList() throws ParserConfigurationException, S
assertEquals("one", matchs.get(0));
assertEquals("two", matchs.get(1));
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

I will take merge test into our current test suite, as it passes with our current code, too :)

@pjfanning pjfanning mentioned this pull request Jul 23, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2022

Codecov Report

Merging #721 (2cecac4) into master (b73f690) will decrease coverage by 0.04%.
The diff coverage is 55.48%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #721      +/-   ##
============================================
- Coverage     55.24%   55.19%   -0.05%     
+ Complexity    10386    10378       -8     
============================================
  Files          1062     1063       +1     
  Lines         65777    65811      +34     
  Branches       7536     7538       +2     
============================================
- Hits          36337    36323      -14     
- Misses        26839    26886      +47     
- Partials       2601     2602       +1     
Impacted Files Coverage Δ
...a/org/apache/jmeter/extractor/XPath2Extractor.java 89.15% <ø> (ø)
...rg/apache/jmeter/functions/XPathFileContainer.java 87.09% <ø> (ø)
.../org/apache/jmeter/util/PrefixResolverDefault.java 10.71% <10.71%> (ø)
...r/util/PropertiesBasedPrefixResolverForXpath2.java 40.00% <11.11%> (-60.00%) ⬇️
...che/jmeter/util/PropertiesBasedPrefixResolver.java 18.60% <11.76%> (-11.03%) ⬇️
...rc/main/java/org/apache/jmeter/util/XPathUtil.java 76.98% <77.27%> (-0.06%) ⬇️
...n/java/org/apache/jmeter/reporters/Summariser.java 90.83% <0.00%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b73f690...2cecac4. Read the comment docs.

@pjfanning
Copy link
Contributor Author

@FSchumacher thanks for reviewing the changes. The build is now working. I've had to make a few changes to get a few existing tests to pass. Would you be able to review the test changes to see if I've made a change that is dangerous?

@pjfanning
Copy link
Contributor Author

I raised JetBrains/lets-plot#576 - I'm not sure if that blocks us from making progress here.

@FSchumacher
Copy link
Contributor

Thanks for your work so far on this. Hopefully I will get time to look more into this. So far it looks good to me.
In my tests, I added code to look at the actual used TransformerFactory and set the used spaces and line-breaks for xalan-like and saxon respectively. But that might really be a bit too much.

@FSchumacher
Copy link
Contributor

I raised JetBrains/lets-plot#576 - I'm not sure if that blocks us from making progress here.

That issue has been resolved and we could use the newer releases to get rid of xalan, when we update our dependencies to the next major version of lets-plot, which has changed the API locations. The changes on our side are a few lines of code.

FSchumacher added a commit to FSchumacher/jmeter that referenced this pull request Mar 26, 2023
This release looses the dependency to xalan, which can help
resolving apache#721
@FSchumacher FSchumacher mentioned this pull request Mar 26, 2023
2 tasks
FSchumacher added a commit to FSchumacher/jmeter that referenced this pull request Mar 26, 2023
This release looses the dependency to xalan, which can help
resolving apache#721
FSchumacher added a commit to FSchumacher/jmeter that referenced this pull request Mar 26, 2023
This release looses the dependency to xalan, which can help
resolving apache#721
vlsi added a commit to FSchumacher/jmeter that referenced this pull request Apr 27, 2023
This release looses the dependency to xalan, which can help
resolving apache#721
vlsi pushed a commit to FSchumacher/jmeter that referenced this pull request Apr 27, 2023
This release looses the dependency to xalan, which can help
resolving apache#721
vlsi pushed a commit that referenced this pull request Apr 27, 2023
This release looses the dependency to xalan, which can help
resolving #721
@vlsi
Copy link
Collaborator

vlsi commented Jul 1, 2023

I see Xalan Java team is not really welcoming contributions. For example, see single-line PRs that say without review, without merge, and even with "not wanted" replies: https://github.com/apache/xalan-java/pulls

So I think we should indeed drop the use of Xalan as they just do not care about the users.

@vlsi vlsi added this to the 6.0 milestone Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants