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.

Sign up for GitHub

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

Jump to bottom

libtest's junit formatter does not produce valid XML for doctests #99813

Open
therealfrauholle opened this issue Jul 27, 2022 · 2 comments
Open

libtest's junit formatter does not produce valid XML for doctests #99813

therealfrauholle opened this issue Jul 27, 2022 · 2 comments
Labels
A-libtest Area: `#[test]` / the `test` library C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-testing-devex Relevant to the testing devex team (testing DX), which will review and decide on the PR/issue.

Comments

@therealfrauholle
Copy link

This issue is related to #85563. The junit formatter of libtest does not produce correct XML for some doctests. Take the following as an example:

use std::marker::PhantomData;


pub struct WithGeneric<A> {
    marker: PhantomData<A>
}

impl<A> WithGeneric<A> {
    /// 
    /// This is a doctest:
    /// 
    /// ```
    /// let _ = hello_world::WithGeneric::<()>::new();
    /// ```
    /// 
    pub fn new() -> Self {
        WithGeneric { marker: PhantomData }
    }
}

When running cargo +nightly-2022-07-26 test --doc -- -Z unstable-options --format junit, the following (invalid) XML is generated (formatted with an external tool):

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
	<testsuite name="test" package="test" id="0" errors="0" failures="0" tests="1" skipped="0" >
		<testcase classname="src/lib.rs" name="WithGeneric<A>::new (line 12)" time="0"/>
		<system-out/>
		<system-err/>
	</testsuite>
</testsuites>

The XML is invalid because of the name attribute of the testcase element. It contains the invalid characters < and >, which should be escaped to &lt; and &gt; as far as I understand.

Proposed solution

In a quick and dirty way I would implement something similar to what the JSON formatter does to escape the strings to valid XML attributes - I just did not have time to make a PR so far.

Apart from that I never contributed to this repository, nor am I specifically familiar with libtest. What would it take to implement the serialization based on serde and a crate like serde_xml_rs? Could I just add these crates as a dependency? I feel like this would be much cleaner than just repeating what the json formatter does.
@therealfrauholle therealfrauholle added the C-bug Category: This is a bug. label Jul 27, 2022
@bjorn3
Copy link
Member

bjorn3 commented Jul 28, 2022

What would it take to implement the serialization based on serde and a crate like serde_xml_rs?

The standard library can't use proc macros as that would break cross-compilation. This means that using serde_derive is not an option. Using serde without it is somewhat awkward. Serialization is less awkward than deserialization, but still somewhat awkward. Looks like serde_xml_rs uses xml-rs for the actual xml parsing and serialization, so using xml-rs directly would be an option, if not for the fact that xml-rs uses the doc_comment proc macro. Other than this, xml-rs is mostly self-contained, so a rewrite to remove the doc_comment proc macro would be possible I think.

@5225225
Copy link
Contributor

5225225 commented Jul 28, 2022

Other than this, xml-rs is mostly self-contained, so a rewrite to remove the doc_comment proc macro would be possible I think.

xml-rs only uses that proc macro for tests. It's to test that the Readme.md's code examples run fine. They'd very likely take a PR to remove it, seeing as it's test only, bumping the MSRV and using #[doc = include("Readme.md")] mod readme_test {} be fine.

However, xml-rs seems unmaintained. So, maybe best not.

Manually implementing XML escaping ourselves would probably be the easiest option. There's also quick-xml. No idea if that uses a proc macro.

@therealfrauholle therealfrauholle mentioned this issue Jun 8, 2023
Open
3 tasks
@fmease fmease added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-libtest Area: `#[test]` / the `test` library T-testing-devex Relevant to the testing devex team (testing DX), which will review and decide on the PR/issue. and removed needs-triage-legacy labels Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-libtest Area: `#[test]` / the `test` library C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-testing-devex Relevant to the testing devex team (testing DX), which will review and decide on the PR/issue.
Projects
testing-devex backlog
Status: No status
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ChrisDenton @5225225 @fmease @bjorn3 @therealfrauholle