diff --git a/library/src/c_api/mod.rs b/library/src/c_api/mod.rs index 815e2a9..d0b435d 100644 --- a/library/src/c_api/mod.rs +++ b/library/src/c_api/mod.rs @@ -503,6 +503,73 @@ mod test { assert_eq!(new, expected_new); } + #[serial] + #[test] + fn current_boot_patch_set_after_reporting_launch_start() { + testing_reset_config(); + let tmp_dir = TempDir::new("example").unwrap(); + + // Generated by `string_patch "hello world" "hello tests"` + let base = "hello world"; + let apk_path = tmp_dir.path().join("base.apk"); + write_fake_apk(apk_path.to_str().unwrap(), base.as_bytes()); + let fake_libapp_path = tmp_dir.path().join("lib/arch/ignored.so"); + let c_params = parameters(&tmp_dir, fake_libapp_path.to_str().unwrap()); + // app_id is required or shorebird_init will fail. + let c_yaml = c_string("app_id: foo"); + assert!(shorebird_init(&c_params, FileCallbacks::new(), c_yaml)); + free_c_string(c_yaml); + free_parameters(c_params); + + // set up the network hooks to return a patch. + testing_set_network_hooks( + |_url, _request| { + // Generated by `string_patch "hello world" "hello tests"` + let hash = "bb8f1d041a5cdc259055afe9617136799543e0a7a86f86db82f8c1fadbd8cc45"; + Ok(PatchCheckResponse { + patch_available: true, + patch: Some(crate::Patch { + number: 1, + hash: hash.to_owned(), + download_url: "ignored".to_owned(), + hash_signature: None, + }), + rolled_back_patch_numbers: None, + }) + }, + |_url| { + // Generated by `string_patch "hello world" "hello tests"` + let patch_bytes: Vec = vec![ + 40, 181, 47, 253, 0, 128, 177, 0, 0, 223, 177, 0, 0, 0, 16, 0, 0, 6, 0, 0, 0, + 0, 0, 0, 5, 116, 101, 115, 116, 115, 0, + ]; + Ok(patch_bytes) + }, + |_url, _event| Ok(()), + ); + + // Ensure we start with no current patch + assert_eq!(shorebird_current_boot_patch_number(), 0); + + // There is an update available. + assert!(shorebird_check_for_update()); + // Go ahead and do the update. + shorebird_update(); + + // Ensure we have not yet updated the current patch. + assert_eq!(shorebird_current_boot_patch_number(), 0); + + shorebird_report_launch_start(); + + // After reporting a launch start, the next boot patch should be the current patch. + assert_eq!(shorebird_current_boot_patch_number(), 1); + + shorebird_report_launch_success(); + + // After reporting a launch success, the current patch number should not have changed. + assert_eq!(shorebird_current_boot_patch_number(), 1); + } + #[serial] #[test] fn forgot_init() { diff --git a/library/src/cache/updater_state.rs b/library/src/cache/updater_state.rs index 88a2758..0930b48 100644 --- a/library/src/cache/updater_state.rs +++ b/library/src/cache/updater_state.rs @@ -171,14 +171,22 @@ impl UpdaterState { self.patch_manager.currently_booting_patch() } - /// This is the current patch that is running. + /// The last patch that was successfully booted (e.g., for which we record_boot_success was + /// called). /// Will be None if: /// - There was no good patch at time of boot. /// - The updater has been initialized but no boot recorded yet. - pub fn current_boot_patch(&self) -> Option { + pub fn last_successfully_booted_patch(&self) -> Option { self.patch_manager.last_successfully_booted_patch() } + /// This is the current patch that is running. + pub fn current_boot_patch(&self) -> Option { + self.patch_manager + .currently_booting_patch() + .or(self.patch_manager.last_successfully_booted_patch()) + } + /// This is the patch that will be used for the next boot. /// Will be None if: /// - There has never been a patch selected. @@ -346,7 +354,7 @@ mod tests { } #[test] - fn current_boot_patch_forwards_from_patch_manager() { + fn last_successfully_booted_patch_forwards_from_patch_manager() { let tmp_dir = TempDir::new("example").unwrap(); let patch = fake_patch(&tmp_dir, 1); let mut mock_manage_patches = MockManagePatches::new(); @@ -354,6 +362,37 @@ mod tests { .expect_last_successfully_booted_patch() .return_const(Some(patch.clone())); let state = test_state(&tmp_dir, mock_manage_patches); + assert_eq!(state.last_successfully_booted_patch(), Some(patch)); + } + + #[test] + fn current_boot_patch_returns_currently_booting_patch_if_present() { + let tmp_dir = TempDir::new("example").unwrap(); + let patch1 = fake_patch(&tmp_dir, 1); + let patch2 = fake_patch(&tmp_dir, 2); + let mut mock_manage_patches = MockManagePatches::new(); + mock_manage_patches + .expect_last_successfully_booted_patch() + .return_const(Some(patch1.clone())); + mock_manage_patches + .expect_currently_booting_patch() + .return_const(Some(patch2.clone())); + let state = test_state(&tmp_dir, mock_manage_patches); + assert_eq!(state.current_boot_patch(), Some(patch2)); + } + + #[test] + fn current_boot_patch_returns_last_successfully_booted_patch_if_no_patch_is_booting() { + let tmp_dir = TempDir::new("example").unwrap(); + let patch = fake_patch(&tmp_dir, 1); + let mut mock_manage_patches = MockManagePatches::new(); + mock_manage_patches + .expect_last_successfully_booted_patch() + .return_const(Some(patch.clone())); + mock_manage_patches + .expect_currently_booting_patch() + .return_const(None); + let state = test_state(&tmp_dir, mock_manage_patches); assert_eq!(state.current_boot_patch(), Some(patch)); } diff --git a/library/src/updater.rs b/library/src/updater.rs index 812ffe4..32d740f 100644 --- a/library/src/updater.rs +++ b/library/src/updater.rs @@ -521,11 +521,6 @@ pub fn next_boot_patch() -> anyhow::Result> { /// The patch that was last successfully booted. If we're booting a patch for the first time, this /// will be the previous patch (or None, if there was no previous patch) until the boot is /// reported as successful. -/// -/// TODO: This should always return the currently running patch, even if it has not been marked as -/// good or bad. Presently, users of the shorebird_code_push package will never get the wrong -/// patch number from this function because a launch will have been reported to be either a -/// success or a failure before they can call this function. pub fn current_boot_patch() -> anyhow::Result> { with_state(|state| Ok(state.current_boot_patch())) } @@ -591,13 +586,17 @@ pub fn report_launch_success() -> anyhow::Result<()> { None => return Ok(()), }; - let maybe_previous_boot_patch = state.current_boot_patch(); + // Get the last successfully booted patch before we record the boot success. + let maybe_previous_boot_patch = state.last_successfully_booted_patch(); state.record_boot_success()?; - if let (Some(previous_boot_patch), Some(current_boot_patch)) = - (maybe_previous_boot_patch, state.current_boot_patch()) - { + // Check whether last_successfully_booted_patch has changed. If so, we should report a + // PatchInstallSuccess event. + if let (Some(previous_boot_patch), Some(current_boot_patch)) = ( + maybe_previous_boot_patch, + state.last_successfully_booted_patch(), + ) { // If we had previously booted from a patch and it has the same number as the // patch we just booted from, then we shouldn't report a patch install. if previous_boot_patch.number == current_boot_patch.number { @@ -1050,7 +1049,10 @@ mod tests { &config.release_version, config.patch_public_key.as_deref(), ); - assert_eq!(state.current_boot_patch().unwrap().number, patch_number); + assert_eq!( + state.last_successfully_booted_patch().unwrap().number, + patch_number + ); Ok(()) }) .unwrap(); @@ -1331,7 +1333,10 @@ mod rollback_tests { report_launch_success()?; with_mut_state(|state| { - assert_eq!(state.current_boot_patch().map(|p| p.number), Some(1)); + assert_eq!( + state.last_successfully_booted_patch().map(|p| p.number), + Some(1) + ); assert_eq!(state.next_boot_patch().map(|p| p.number), Some(1)); Ok(()) })?; @@ -1432,7 +1437,10 @@ mod rollback_tests { report_launch_success()?; with_mut_state(|state| { - assert_eq!(state.current_boot_patch().map(|p| p.number), Some(2)); + assert_eq!( + state.last_successfully_booted_patch().map(|p| p.number), + Some(2) + ); assert_eq!(state.next_boot_patch().map(|p| p.number), Some(2)); Ok(()) })?;