Add tests for the expiry date calculation.

[wt]

This is not pretty. I needed to make the test deterministic, so I just hacked in a fixed time for the current time when in test mode only. Hopefully, we can iterate and make this less ugly.

[maxcountryman]

Is there a reason to not use MemoryStore here? One of its primary reasons for existing is testing so that mocks can be avoided.

[wt]

I wanted a mocked store that I could explicitly set returns for. I don't want to depend on semantics of MemoryStore.

[wt]

Also, I think it's important not to have a circular dependency for unit tests.

MemoryStore is more appropriate for integration testing.

[maxcountryman]

I think a better approach here is to build in some tolerance for drift. For example:

tower-sessions/tests/common/mod.rs

Lines 374 to 383 in bc0d0f9

	let expected_duration = Duration::days(1);
	let actual_duration = session_cookie.max_age().unwrap();
	let tolerance = Duration::seconds(1);
	assert!(
	actual_duration >= expected_duration - tolerance
	&& actual_duration <= expected_duration + tolerance,
	"Duration is not within the acceptable range: {:?}",
	actual_duration
	);

We could wrap this with an assert macro of our own to make it a little more ergonomic to use in the tests.

[wt]

I think it's more important to have determinism in tests. I think that I can extract that into something that looks less offensive. I was just doing as little as possible to show the concept.

I think what should really happen is that we should have a function for returning the current time that is what gets mocked in tests. Production would defer to the time module.

Doing it this way would allow determinism in the tests so that there is no slop. It's basically mocking a time source. I think this ends up being much cleaner since it's just assert_eq that everyone understands and had simple semantics. If it fails, I trust this a lot more than trying to capture a "real" time source and changing inputs on every test run.

[wt]

I refactored the mock to outside of the functions that use now_utc so that it wouldn't be so aesthetically offensive.

[wt]

To add more context on using MemoryStore. The semantics of MemoryStore are not guaranteed to be implemented without bugs or with specific semantics. As an example, MemoryStore will currently allow a colliding session id to be saved over a currently existing id in the store without any kind of signal that that has happened. I don't want to have to tease out if that semantic effects that logic of my tests.

Rather than have to tease all those semantics out and make sure that the test and the dependent code implement that, I think it makes more sense to mock it out and force the semantics that we want. This is a relatively simple case of that, but I think it's useful to be able to say that I don't have to read anything other than the test function to understand what the expectations are vs. having to read the code for the MemoryStore to understand how the code should work. In one sense, I don't want to have to know what MemoryStore does to troubleshoot a failing test.

As such, I prefer to isolate the code as much as possible during a unit test and to leave integration testing to the specific integration test layer of tests. This will capture errors that may not be identified if we only have the integration level of testing. With this approach, a succeeding unit test and a failing integration test indicate that something is wrong with the way MemoryStore is being used. However, I would know that the expectations of the memory store interface are not being violated. This would allow me to focus on the actual problem instead of having to figure that out first.

I hope all this makes sense. I know I am not the most terse communicator. :)

[wt]

Oops. Forgot to format the code. Done now.

[wt]

FWIW, I also thought about using mockall to mock out the time module, but that seemed a bit overkill in this case when I only needed to mock one function.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.86%. Comparing base (bc0d0f9) to head (accd8ff).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #179      +/-   ##
==========================================
+ Coverage   80.37%   80.86%   +0.49%     
==========================================
  Files           5        5              
  Lines         321      324       +3     
==========================================
+ Hits          258      262       +4     
+ Misses         63       62       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[wt]

One more push to fixup the commit message.

[wt]

The apparent reduction in coverage is because of the fact that I can't cover the non-test version of the from_utc function. I think it would probably be best to isolate these uncoverable functions to a module that only has pub(crate) functions and has well know semantics on the coverage expectations instead of embedding them in various different modules. However, I did not do that here since that infra doesn't already exist.

[maxcountryman]

Rather than creating visual noise in the library code, I opted to create mocks directly in the test modulate here:

tower-sessions/tower-sessions-core/src/session_store.rs

Lines 301 to 337 in 6ad8933

	mock! {
	#[derive(Debug)]
	pub Cache {}
	#[async_trait]
	impl SessionStore for Cache {
	async fn create(&self, record: &mut Record) -> Result<()>;
	async fn save(&self, record: &Record) -> Result<()>;
	async fn load(&self, session_id: &Id) -> Result<Option<Record>>;
	async fn delete(&self, session_id: &Id) -> Result<()>;
	}
	}
	mock! {
	#[derive(Debug)]
	pub Store {}
	#[async_trait]
	impl SessionStore for Store {
	async fn create(&self, record: &mut Record) -> Result<()>;
	async fn save(&self, record: &Record) -> Result<()>;
	async fn load(&self, session_id: &Id) -> Result<Option<Record>>;
	async fn delete(&self, session_id: &Id) -> Result<()>;
	}
	}
	mock! {
	#[derive(Debug)]
	pub CollidingStore {}
	#[async_trait]
	impl SessionStore for CollidingStore {
	async fn save(&self, record: &Record) -> Result<()>;
	async fn load(&self, session_id: &Id) -> Result<Option<Record>>;
	async fn delete(&self, session_id: &Id) -> Result<()>;
	}
	}

This also has the benefit of allowing for alterations of the mocks (as seen here with the colliding store, for example).

[wt]

My intention was to avoid the duplicate interface definitions. However, I am not gonna tell you how to run your project.

This seems fine.

[maxcountryman]

This is a good opportunity to break this apart into its own function to better facilitate testing.

For example, something along these lines:

diff --git a/tower-sessions-core/src/session.rs b/tower-sessions-core/src/session.rs
index ad0cd93..dfa49d2 100644
--- a/tower-sessions-core/src/session.rs
+++ b/tower-sessions-core/src/session.rs
@@ -551,14 +551,18 @@ impl Session {
     /// assert!(session.expiry_date() < expected_expiry.saturating_add(Duration::seconds(1)));
     /// ```
     pub fn expiry_date(&self) -> OffsetDateTime {
+        self.expiry_date_with_now(OffsetDateTime::now_utc())
+    }
+
+    fn expiry_date_with_now(&self, now: OffsetDateTime) -> OffsetDateTime {
         let expiry = self.expiry.lock();
         match *expiry {
-            Some(Expiry::OnInactivity(duration)) => {
-                OffsetDateTime::now_utc().saturating_add(duration)
-            }
+            Some(Expiry::OnInactivity(duration)) => now.saturating_add(duration),
             Some(Expiry::AtDateTime(datetime)) => datetime,
             Some(Expiry::OnSessionEnd) | None => {
-                OffsetDateTime::now_utc().saturating_add(DEFAULT_DURATION) // TODO: The default should probably be configurable.
+                now.saturating_add(DEFAULT_DURATION) // TODO: The default should
+                                                     // probably be
+                                                     // configurable.
             }
         }
     }

[wt]

Seems like you are suggesting a dep injection style. Again, I have no problem with that. I was just trying to do the minimum.

Again, this is fine.

[wt]

If this PR isn't heading in a direction you want, I don't have a strong opinion. I honestly just want a working session cookie framework. I am happy to defer to your preferences on this.

I need to get back to other work, so I am probably not going to iterate any more on this change.

Thanks again for the work you did. I appreciate your putting this lib together.

[maxcountryman]

If this PR isn't heading in a direction you want, I don't have a strong opinion.

I'm okay to merge it once the comments are addressed. That's:

1. Injection of test state (so now for the test case in this PR)
2. If mocks are necessary, then I'd prefer we use them explicitly as opposed to decorating the library code with a proc macro (following the same style as the test mod I linked)

I don't think those are big changes, but no worries if this isn't something you want to pursue or otherwise don't have time for.

It's also worth mentioning we have strong coverage of this interface via the doc tests so hopefully that helps give you confidence in the crate (overall we now have ~85% coverage of the crate).