[Merged by Bors] - Make `Time::update_with_instant` public for use in tests by mdickopp · Pull Request #4469 · bevyengine/bevy

mdickopp · 2022-04-13T18:58:59Z

Objective

To test systems that implement frame rate-independent update logic, one needs to be able to mock Time. By mocking time, it's possible to write tests that confirm systems are frame rate-independent.

This is a follow-up PR to #2549 by @ostwilkens and based on his work.

Solution

To mock Time, one needs to be able to manually update the Time resource with an Instant defined by the developer. This can be achieved by making the existing Time::update_with_instant method public for use in tests.

Changelog

Make Time::update_with_instant public
Add doc to Time::update_with_instant clarifying that the method should not be called outside of tests.
Add doc test to Time demonstrating how to use update_with_instant in tests.

mockersf · 2022-04-13T21:49:49Z

crates/bevy_core/src/time/time.rs

+    /// 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) {


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

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

#[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?

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

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

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

crates/bevy_core/src/time/time.rs

mdickopp · 2022-04-15T09:27:48Z

I see two options, either leave the patch unchanged, or add the #[cfg(test)] attribute and remove the doctest. Please let me know what you prefer.

Instead of removing the code in the doctest entirely, I would transform it into an example.

alice-i-cecile · 2022-04-15T19:49:42Z

My preference is for the former: I don't think the API is so dangerous that we need to hide it.

mdickopp · 2022-04-21T17:10:22Z

@mockersf, do you require code changes?

alice-i-cecile · 2022-04-23T04:09:05Z

@maniwani are you on board with merging this? I saw you mention it on Discord, and want your opinion.

maniwani

@maniwani are you on board with merging this?

Yeah, I'm cool with making this method pub. Nothing was stopping someone from calling Time::update before.

A couple tiny nits. Also you should probably have the note saying the CorePlugin is usually in charge on the update doc as well.

maniwani · 2022-04-23T04:22:01Z

crates/bevy_core/src/time/time.rs

+    /// 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.


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).

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

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

Good idea, agreed.

Sure. Done!

maniwani · 2022-04-23T04:23:57Z

crates/bevy_core/src/time/time.rs

+/// ```
+/// # 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);
+/// }
+/// ```


I suggest moving this doc test to update_with_instant.

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

mdickopp · 2022-04-24T05:52:00Z

I'm confused by the CI failure, as I didn't change any markdown documents. Any advice would be appreciated.

alice-i-cecile · 2022-04-24T05:54:31Z

This is just a flaky CI failure @mdickopp, don't worry about it.

alice-i-cecile · 2022-04-24T05:54:58Z

bors r+

@ostwilkens

# Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to #2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

bors · 2022-04-24T05:59:51Z

Build failed:

check-markdown-links

alice-i-cecile · 2022-04-24T06:08:47Z

bors retry

@ostwilkens

# Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to #2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

bors · 2022-04-24T06:13:34Z

Build failed:

check-markdown-links

alice-i-cecile · 2022-04-24T18:52:18Z

bors r+

@ostwilkens

# Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to #2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

bors · 2022-04-24T18:57:17Z

Build failed:

check-markdown-links

alice-i-cecile · 2022-04-24T20:33:49Z

bors r+

@ostwilkens

# Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to #2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

bors · 2022-04-24T22:32:47Z

Timed out.

alice-i-cecile · 2022-04-24T22:57:23Z

bors r+

@ostwilkens

# Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to #2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

bors · 2022-04-24T23:32:57Z

Pull request successfully merged into main.

Build succeeded:

mdickopp · 2022-04-25T07:45:54Z

Thanks!

@ostwilkens

…4469) # Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to bevyengine#2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

@ostwilkens

…4469) # Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to bevyengine#2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org> (cherry picked from commit 93cee3b)

@ostwilkens

…4469) # Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to bevyengine#2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

ostwilkens and others added 3 commits April 13, 2022 18:29

Make Time update_with_instant pub

4fbf17f

Add documentation to Time::update_with_instant

84c7310

Add doc test demonstrating how to mock Time in tests

04288ad

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 13, 2022

mdickopp mentioned this pull request Apr 13, 2022

Make Time update_with_instant pub #2549

Closed

alice-i-cecile added A-Core C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Apr 13, 2022

alice-i-cecile self-requested a review April 13, 2022 19:30

mockersf reviewed Apr 13, 2022

View reviewed changes

alice-i-cecile reviewed Apr 13, 2022

View reviewed changes

crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved

alice-i-cecile approved these changes Apr 13, 2022

View reviewed changes

mdickopp changed the title ~~Make Time::update_with_instant public for use in tests~~ Make Time::update_with_instant public for use in tests Apr 17, 2022

maniwani reviewed Apr 23, 2022

View reviewed changes

mdickopp added 2 commits April 23, 2022 14:06

Improve wording of Time::update_with_instant documentation

726e04b

Move doctest from Time to Time::update_with_instant

2fa1fb9

alice-i-cecile approved these changes Apr 23, 2022

View reviewed changes

Add warning to Time::update doc

468029b

alice-i-cecile mentioned this pull request Apr 24, 2022

check-markdown-links is painfully flaky #4575

Closed

IceSentry approved these changes Apr 24, 2022

View reviewed changes

maniwani approved these changes Apr 24, 2022

View reviewed changes

bors bot changed the title ~~Make Time::update_with_instant public for use in tests~~ [Merged by Bors] - Make Time::update_with_instant public for use in tests Apr 24, 2022

bors bot closed this Apr 24, 2022

mdickopp deleted the pub_update_with_instant branch April 25, 2022 07:47

CAD97 mentioned this pull request Apr 26, 2022

[Merged by Bors] - Split time functionality into bevy_time #4187

Closed

Uh oh!

Conversation

mdickopp commented Apr 13, 2022

Objective

Solution

Changelog

Uh oh!

mockersf Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdickopp commented Apr 15, 2022

Uh oh!

alice-i-cecile commented Apr 15, 2022

Uh oh!

mdickopp commented Apr 21, 2022

Uh oh!

alice-i-cecile commented Apr 23, 2022

Uh oh!

maniwani left a comment

Choose a reason for hiding this comment

Uh oh!

maniwani Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maniwani Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdickopp commented Apr 24, 2022

Uh oh!

alice-i-cecile commented Apr 24, 2022

Uh oh!

alice-i-cecile commented Apr 24, 2022

Uh oh!

bors bot commented Apr 24, 2022

Uh oh!

alice-i-cecile commented Apr 24, 2022

Uh oh!

bors bot commented Apr 24, 2022

Uh oh!

alice-i-cecile commented Apr 24, 2022

Uh oh!

bors bot commented Apr 24, 2022

Uh oh!

alice-i-cecile commented Apr 24, 2022

Uh oh!

bors bot commented Apr 24, 2022

Uh oh!

alice-i-cecile commented Apr 24, 2022

Uh oh!

bors bot commented Apr 24, 2022

Uh oh!

mdickopp commented Apr 25, 2022

Uh oh!

Reviewers

Assignees

Labels

mockersf Apr 13, 2022 •

edited

Loading

maniwani Apr 23, 2022 •

edited

Loading

maniwani Apr 23, 2022 •

edited

Loading