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

[Merged by Bors] - Make Time::update_with_instant public for use in tests #4469

Closed
wants to merge 6 commits into from
Closed
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion crates/bevy_core/src/time/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,52 @@ use bevy_ecs::system::ResMut;
use bevy_utils::{Duration, Instant};

/// Tracks elapsed time since the last update and since the App has started
///
/// # Examples
mdickopp marked this conversation as resolved.
Show resolved Hide resolved
///
/// ```
/// # use bevy_core::prelude::*;
/// # use bevy_ecs::prelude::*;
/// # use bevy_utils::Duration;
/// # fn main () {
/// # test_health_system();
/// # }
/// struct Health {
/// // Health value between 0.0 and 1.0
/// health_value: f32,
/// }
///
/// fn health_system(time: Res<Time>, mut health: ResMut<Health>) {
/// // Increase health value by 0.1 per second, independent of frame rate,
/// // but not beyond 1.0
/// health.health_value = (health.health_value + 0.1 * time.delta_seconds()).min(1.0);
/// }
///
/// // Mock time in tests
/// fn test_health_system() {
/// let mut world = World::default();
/// let mut time = Time::default();
/// time.update();
/// world.insert_resource(time);
/// world.insert_resource(Health { health_value: 0.2 });
///
/// let mut update_stage = SystemStage::single_threaded();
/// update_stage.add_system(health_system);
///
/// // Simulate that 30 ms have passed
/// let mut time = world.get_resource_mut::<Time>().unwrap();
/// let last_update = time.last_update().unwrap();
/// time.update_with_instant(last_update + Duration::from_millis(30));
///
/// // Run system
/// update_stage.run(&mut world);
///
/// // Check that 0.003 has been added to the health value
/// let expected_health_value = 0.2 + 0.1 * 0.03;
/// let actual_health_value = world.get_resource::<Health>().unwrap().health_value;
/// assert_eq!(expected_health_value, actual_health_value);
/// }
/// ```
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 moving this doc test to update_with_instant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the doc test,and I also replaced world.get_resource_mut::<Time>().unwrap() with the simpler world.resource_mut::<Time>().

#[derive(Debug, Clone)]
pub struct Time {
delta: Duration,
Expand Down Expand Up @@ -33,7 +79,12 @@ impl Time {
self.update_with_instant(Instant::now());
}

pub(crate) fn update_with_instant(&mut self, instant: Instant) {
/// Update time with a specified [`Instant`]
///
/// This method is for use in tests, and should generally not be called on the [`Time`] resource
/// as part of your app. Doing so will conflict with the `time_system` called by
/// [`CorePlugin`](crate::CorePlugin), and result in inaccurate performance measurements.
Copy link
Contributor

@maniwani maniwani Apr 23, 2022

Choose a reason for hiding this comment

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

Suggested change
/// This method is for use in tests, and should generally not be called on the [`Time`] resource
/// as part of your app. Doing so will conflict with the `time_system` called by
/// [`CorePlugin`](crate::CorePlugin), and result in inaccurate performance measurements.
/// This method is provided for use in tests. Calling this method on the [`Time`] resource as part of your app
/// will most likely result in inaccurate timekeeping, as the resource is ordinarily managed by the [`CorePlugin`](crate::CorePlugin).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion! I updated the documentation accordingly.

Copy link
Contributor

@maniwani maniwani Apr 23, 2022

Choose a reason for hiding this comment

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

Can you add the second sentence ("Calling this...") as a disclaimer to Time::update as well?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea, agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done!

pub fn update_with_instant(&mut self, instant: Instant) {
Copy link
Member

@mockersf mockersf Apr 13, 2022

Choose a reason for hiding this comment

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

Suggested change
/// Update time with a specified [`Instant`]
///
/// This method is for use in tests, and should generally not be called on the [`Time`] resource
/// as part of your app. Doing so will conflict with the `time_system` called by
/// [`CorePlugin`](crate::CorePlugin), and result in inaccurate performance measurements.
pub fn update_with_instant(&mut self, instant: Instant) {
/// Update time with a specified [`Instant`]
///
/// This method is for use in tests, and should generally not be called on the [`Time`] resource
/// as part of your app. Doing so will conflict with the `time_system` called by
/// [`CorePlugin`](crate::CorePlugin), and result in inaccurate performance measurements.
#[cfg(test)]
pub fn manual_update_with_instant(&mut self, instant: Instant) {
self.update_with_instant(instant);
}
pub(crate) fn update_with_instant(&mut self, instant: Instant) {

not sure this is a good idea, but it would make it impossible to mess with time. Doc comment above would need to be updated too

Copy link
Member

Choose a reason for hiding this comment

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

TBH I don't think this actually adds much more safety. I think that aggressive doc warnings should be sufficient here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#[cfg(test)] hides the method from the new doctest. And #[cfg(doctest)] does not work here due to a bug in cargo (rust-lang/rust#67295). Any ideas how this could be solved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mockersf, I agree with @alice-i-cecile. Is it acceptable to you to leave the patch unchanged?

Copy link
Contributor

Choose a reason for hiding this comment

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

#[cfg(test)] only applies when testing the current crate, not when testing a dependent crate.

Copy link
Member

Choose a reason for hiding this comment

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

It's acceptable, even more with Bjorn3 remark! (we already had this discussion on another place, and I keep forgetting that...).

It's not something I wanted to block on anyway

if let Some(last_update) = self.last_update {
self.delta = instant - last_update;
self.delta_seconds_f64 = self.delta.as_secs_f64();
Expand Down