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

Various fixes to allow testing with no_std #1177

Closed
wants to merge 15 commits into from

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Jul 8, 2023

lib.rs has an interesting line: #![cfg_attr(not(any(feature = "std", test)), no_std)].
So std is always available while testing, even with --no-default-features.
I would like to test our no_std support truly works without std.

Some fixes are just a matter of correcting imports or using feature gates.

The more interesting ones are:

  • Compare the parsing of a format string with the expected items while iterating in test_strftime_items.
  • I am quite pleased with the alternative to using assert_eq!(value.to_string(), "expected") or assert_eq!(format!("{}", value), "expected").
    We add a type that implements fmt::Write and knows expected. If it receives a byte that does not match expected it returns an error. This is used in the assert_debug_eq and assert_display_eq functions.
  • We were missing feature gates in src/oldtime.rs that should have been added in Feature gate tests instead of methods #1159. We should have a good look at the checks the CI runs at some point.

I have verified that all the tests we skip with --no-default-features really require allocations or another feature from std.

@pitdicker
Copy link
Collaborator Author

cc @jtmoon79 I know no one who is as interested in tests 😄

@pitdicker pitdicker force-pushed the no_std_tests branch 2 times, most recently from 6bcfd6c to d766f39 Compare July 8, 2023 07:49
@pitdicker pitdicker changed the title Various fixed to allow testing with no_std Various fixes to allow testing with no_std Jul 8, 2023
src/utils.rs Outdated
} else {
#[cfg(feature = "std")]
eprintln!(
"formatting difference: `(left == right)`\n left: `\"{}{}(...)\"`\n right: `\"{}\"`",
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using {:?} here so formatting characters like newline and tab are escaped. You can remove the explicit " in the string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't know that! 👍
The "'s are to imitate the format of the assert_eq! macro.

})
.join()
.unwrap();
fn test_datetime_is_send_and_copy() {
Copy link
Contributor

@jtmoon79 jtmoon79 Jul 9, 2023

Choose a reason for hiding this comment

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

hmm... this is beyond the edge of my rust knowledge.
Does this test test_datetime_is_send_and_copy invoke trait Send on a Utc? And trait Copy?
I can see function _assert_send_copy requires T to have those traits, but AFAICT, those traits are not exercised here. Previously, those traits were exercised.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@jtmoon79 jtmoon79 Jul 9, 2023

Choose a reason for hiding this comment

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

I created a little example of the old code here that attempts to invoke the Copy trait. Surprisingly, I couldn't get the underlying call to clone() to print anything (I'm pretty sure clone() is invoked by doing a Copy) so I'm not sure what's happening.

fn main() {
    println!("Hello, world!");
}

#[derive(Debug, PartialEq, Eq)]
pub struct Object2 {}
impl Object2 {
    pub fn new() -> Self {
        Object2 {}
    }
}
impl Clone for Object2 {
    fn clone(&self) -> Object2 {
        eprintln!("Clone");
        *self
    }
}
impl Copy for Object2 {}

#[test]
fn test_copy() {
    eprintln!("test_copy start...");
    let a = Object2::new();
    let b = a;
    assert_eq!(a, b);
}

Run with cargo test -v -- --nocapture

I guess your change is probably okay... 🤔

Copy link
Collaborator Author

@pitdicker pitdicker Jul 9, 2023

Choose a reason for hiding this comment

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

It works the other way around. Clone may use a simple memcpy, but Copy is guaranteed to be just a copy and not call any functions.

src/utils.rs Outdated

#[cfg(test)]
#[track_caller]
pub(crate) fn assert_display_eq<D>(value: D, expected: &str)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a docstring comment here describing why assert_display_eq was implemented. As a typical code reader, I would be confused why chrono goes through so much work to implement this.

/// Implement the rough equivalent of `assert_eq!` for values with the `Display` trait that
/// does not use `std` functions.
/// This allows chrono tests to ensure chrono behavior when `no_std` is in effect.

I don't know if my made-up explanation is accurate but you get the idea.

src/utils.rs Outdated

#[cfg(test)]
#[track_caller]
pub(crate) fn assert_debug_eq<D>(value: D, expected: &str)
Copy link
Contributor

Choose a reason for hiding this comment

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

This also needs a docstring explanation as discussed above for assert_display_eq.

}

#[cfg(test)]
impl<'a> Write for WriteCompare<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This also needs a docstring explanation as discussed below for assert_display_eq.

@pitdicker pitdicker force-pushed the no_std_tests branch 3 times, most recently from 8f6dccb to 2f186cd Compare July 15, 2023 06:34
Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

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

LGTM

Overridding various common assert* macros does add some mental overhead to tests. However, this is thorough about testing no_std which is good to do.

@pitdicker pitdicker force-pushed the no_std_tests branch 2 times, most recently from 4ce394f to ac090c4 Compare July 17, 2023 14:50
@pitdicker
Copy link
Collaborator Author

pitdicker commented Jul 18, 2023

If I look at this PR honestly, it is better to close it.

The test harness relies on the standard library, so we have that available anyway.
I still think it is cool to be able to run the tests without needing any allocations 😄.
But it means needlessly restricting our test suite.

I'll open an other PR with the 'good parts'.

😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants