Skip to content

Conversation

@edsavage
Copy link
Contributor

@edsavage edsavage commented Jan 7, 2025

Replacing the use of XML serialisers in unit tests with equivalent JSON serialisers allows the removal of dependencies on the 3rd party RapidXML library and also the bespoke XML parser and related code.

Labelling the PR as >non-issue as despite it's size it doesn't affect any external code or results.

Closes #2804

Replacing the use of XML serialisers in unit tests with equivalent JSON serialisers allows the removal of dependencies on the 3rd party RapidXML library and also the bespoke XML parser and related code.

Closes elastic#2804
@edsavage edsavage marked this pull request as draft January 7, 2025 19:50
@edsavage edsavage marked this pull request as ready for review January 9, 2025 23:43
@edsavage edsavage requested a review from valeriy42 January 9, 2025 23:44
Copy link
Contributor

@valeriy42 valeriy42 left a comment

Choose a reason for hiding this comment

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

Good work! Does this mean that we can now remove libxml2 as our dependency? If so, we probably also need to adjust the CMake files and the 3rd party license info.

core_t::TTime data1[],
core_t::TTime data2[],
std::string expectedPersonCounts[],
SModelParams params,
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be a const reference?


void testGathererMultipleSeries(const core_t::TTime startTime,
const core_t::TTime bucketLength,
SModelParams params,
Copy link
Contributor

Choose a reason for hiding this comment

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

params is not used in the function.

@edsavage
Copy link
Contributor Author

Good work! Does this mean that we can now remove libxml2 as our dependency? If so, we probably also need to adjust the CMake files and the 3rd party license info.

I think so. My only hesitation is that it might be needed by one of the boost libraries. I'll check on all platforms before adjusting CMake, docs, build scripts etc.

@edsavage
Copy link
Contributor Author

Good work! Does this mean that we can now remove libxml2 as our dependency? If so, we probably also need to adjust the CMake files and the 3rd party license info.

I think so. My only hesitation is that it might be needed by one of the boost libraries. I'll check on all platforms before adjusting CMake, docs, build scripts etc.

We still have a dependency on libxml2 because we're using it to generate XML formatted Boost Test results - see lib/test/CBoostTestXmlOutput.cc. I think we want to keep it as-is since it's much easier for a human to read than the JUNIT output we also generate (for the Buildkite Test Analytics)

Copy link
Contributor

@valeriy42 valeriy42 left a comment

Choose a reason for hiding this comment

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

LGTM. Also, thank you for including the SonarQube linting suggestions.

@elastic-sonarqube
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
5.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube

@edsavage edsavage merged commit 573abc6 into elastic:main Jan 14, 2025
14 of 15 checks passed
@edsavage edsavage deleted the remove_xml_parsers branch March 20, 2025 03:45
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.

Remove XML dependencies

2 participants