Write tests and ensure adding instances works

rojo-insta-ext/src/redaction_map.rs

@@ -18,6 +18,16 @@ impl RedactionMap {
         }
     }
 
+    pub fn get_redacted_value(&self, id: impl ToString) -> Option<String> {
+        let id = id.to_string();
+
+        if self.ids.contains_key(&id) {
+            Some(id)
+        } else {
+            None
+        }
+    }
+
     pub fn intern(&mut self, id: impl ToString) {
         let last_id = &mut self.last_id;
 

rojo-test/serve-test-snapshots/serve_test__add_folder_all-2.snap

@@ -0,0 +1,26 @@
+---
+source: rojo-test/src/serve_test.rs
+expression: "read_response.intern_and_redact(&mut redactions, root_id)"
+---
+instances:
+  id-2:
+    Id: id-2
+    Parent: ~
+    Name: add_folder
+    ClassName: Folder
+    Properties: {}
+    Children:
+      - id-3
+    Metadata:
+      ignoreUnknownInstances: false
+  id-3:
+    Id: id-3
+    Parent: id-2
+    Name: my-new-folder
+    ClassName: Folder
+    Properties: {}
+    Children: []
+    Metadata:
+      ignoreUnknownInstances: false
+messageCursor: 1
+sessionId: id-1

rojo-test/serve-test-snapshots/serve_test__add_folder_all.snap

@@ -0,0 +1,16 @@
+---
+source: rojo-test/src/serve_test.rs
+expression: "read_response.intern_and_redact(&mut redactions, root_id)"
+---
+instances:
+  id-2:
+    Id: id-2
+    Parent: ~
+    Name: add_folder
+    ClassName: Folder
+    Properties: {}
+    Children: []
+    Metadata:
+      ignoreUnknownInstances: false
+messageCursor: 0
+sessionId: id-1

rojo-test/serve-test-snapshots/serve_test__add_folder_info.snap

@@ -0,0 +1,9 @@
+---
+source: rojo-test/src/serve_test.rs
+expression: redactions.redacted_yaml(info)
+---
+expectedPlaceIds: ~
+protocolVersion: 3
+rootInstanceId: id-2
+serverVersion: 0.6.0-dev
+sessionId: id-1

rojo-test/serve-test-snapshots/serve_test__add_folder_subscribe.snap

@@ -0,0 +1,19 @@
+---
+source: rojo-test/src/serve_test.rs
+expression: "subscribe_response.intern_and_redact(&mut redactions, ())"
+---
+messageCursor: 1
+messages:
+  - removedInstances: []
+    addedInstances:
+      id-3:
+        Id: id-3
+        Parent: id-2
+        Name: my-new-folder
+        ClassName: Folder
+        Properties: {}
+        Children: []
+        Metadata:
+          ignoreUnknownInstances: false
+    updatedInstances: []
+sessionId: id-1

rojo-test/src/internable.rs

@@ -0,0 +1,104 @@
+use std::collections::HashMap;
+
+use rbx_dom_weak::RbxId;
+use serde::Serialize;
+
+use librojo::web_interface::{Instance, InstanceUpdate, ReadResponse, SubscribeResponse};
+use rojo_insta_ext::RedactionMap;
+
+/// A convenience method to store all of the redactable data from a piece of
+/// data, then immediately redact it and return a serde_yaml Value.
+pub trait InternAndRedact<T> {
+    fn intern_and_redact(&self, redactions: &mut RedactionMap, extra: T) -> serde_yaml::Value;
+}
+
+impl<I, T> InternAndRedact<T> for I
+where
+    I: Serialize + Internable<T>,
+{
+    fn intern_and_redact(&self, redactions: &mut RedactionMap, extra: T) -> serde_yaml::Value {
+        self.intern(redactions, extra);
+        redactions.redacted_yaml(self)
+    }
+}
+
+/// A trait to describe how to discover redactable data from an type.
+///
+/// The 'extra' parameter is a kludge to support types like Instance or
+/// ReadResponse that need some additional information in order to be
+/// deterministic.
+pub trait Internable<T> {
+    fn intern(&self, redactions: &mut RedactionMap, extra: T);
+}
+
+impl Internable<RbxId> for ReadResponse<'_> {
+    fn intern(&self, redactions: &mut RedactionMap, root_id: RbxId) {
+        redactions.intern(root_id);
+
+        let root_instance = self.instances.get(&root_id).unwrap();
+
+        for &child_id in root_instance.children.iter() {
+            self.intern(redactions, child_id);
+        }
+    }
+}
+
+impl<'a> Internable<&'a HashMap<RbxId, Instance<'_>>> for Instance<'a> {
+    fn intern(
+        &self,
+        redactions: &mut RedactionMap,
+        other_instances: &HashMap<RbxId, Instance<'_>>,
+    ) {
+        redactions.intern(self.id);
+
+        for child_id in self.children.iter() {
+            let child = &other_instances[child_id];
+            child.intern(redactions, other_instances);
+        }
+    }
+}
+
+impl Internable<()> for SubscribeResponse<'_> {
+    fn intern(&self, redactions: &mut RedactionMap, _extra: ()) {
+        for message in &self.messages {
+            intern_instance_updates(redactions, &message.updated_instances);
+            intern_instance_additions(redactions, &message.added_instances);
+        }
+    }
+}
+
+fn intern_instance_updates(redactions: &mut RedactionMap, updates: &[InstanceUpdate]) {
+    for update in updates {
+        redactions.intern(update.id);
+    }
+}
+
+fn intern_instance_additions(
+    redactions: &mut RedactionMap,
+    additions: &HashMap<RbxId, Instance<'_>>,
+) {
+    // This method redacts in a deterministic order from a HashMap by collecting
+    // all of the instances that are direct children of instances we've already
+    // interned.
+    let mut added_roots = Vec::new();
+
+    for (id, added) in additions {
+        let parent_id = added.parent.unwrap();
+        let parent_redacted = redactions.get_redacted_value(parent_id);
+
+        // Here, we assume that instances are only added to other instances that
+        // we've already interned. If that's not true, then we'll have some
+        // dangling unredacted IDs.
+        if let Some(parent_redacted) = parent_redacted {
+            added_roots.push((id, parent_redacted));
+        }
+    }
+
+    // Sort the input by the redacted key, which should match the traversal
+    // order we need for the tree.
+    added_roots.sort_unstable_by(|a, b| a.1.cmp(&b.1));
+
+    for (root_id, _redacted_id) in added_roots {
+        additions[root_id].intern(redactions, additions);
+    }
+}

rojo-test/src/lib.rs

@@ -1,8 +1,7 @@
 #![cfg(test)]
 
-#[macro_use]
-mod serve_util;
-
 mod build_test;
+mod internable;
 mod serve_test;
+mod serve_util;
 mod util;

rojo-test/src/serve_test.rs

@@ -2,7 +2,7 @@ use std::fs;
 
 use insta::assert_yaml_snapshot;
 
-use crate::serve_util::{run_serve_test, InternAndRedact};
+use crate::{internable::InternAndRedact, serve_util::run_serve_test};
 
 #[test]
 fn empty() {
@@ -39,7 +39,7 @@ fn scripts() {
         let subscribe_response = session.get_api_subscribe(0).unwrap();
         assert_yaml_snapshot!(
             "scripts_subscribe",
-            redactions.redacted_yaml(subscribe_response)
+            subscribe_response.intern_and_redact(&mut redactions, ())
         );
 
         let read_response = session.get_api_read(root_id).unwrap();
@@ -70,9 +70,7 @@ fn just_txt() {
     });
 }
 
-// TODO: Rojo doesn't currently handle files/folders being added.
 #[test]
-#[ignore]
 fn add_folder() {
     run_serve_test("add_folder", |session, mut redactions| {
         let info = session.get_api_rojo().unwrap();
@@ -91,7 +89,7 @@ fn add_folder() {
         let subscribe_response = session.get_api_subscribe(0).unwrap();
         assert_yaml_snapshot!(
             "add_folder_subscribe",
-            redactions.redacted_yaml(subscribe_response)
+            subscribe_response.intern_and_redact(&mut redactions, ())
         );
 
         let read_response = session.get_api_read(root_id).unwrap();

rojo-test/src/serve_util.rs

@@ -3,10 +3,12 @@ use std::{
     path::{Path, PathBuf},
     process::Command,
     sync::atomic::{AtomicUsize, Ordering},
+    thread,
+    time::Duration,
 };
 
 use rbx_dom_weak::RbxId;
-use serde::Serialize;
+
 use tempfile::{tempdir, TempDir};
 
 use librojo::web_interface::{ReadResponse, ServerInfoResponse, SubscribeResponse};
@@ -16,6 +18,13 @@ use crate::util::{
     copy_recursive, get_rojo_path, get_serve_tests_path, get_working_dir_path, KillOnDrop,
 };
 
+/// Convenience method to run a `rojo serve` test.
+///
+/// Test projects should be defined in the `serve-tests` folder; their filename
+/// should be given as the first parameter.
+///
+/// The passed in callback is where the actual test body should go. Setup and
+/// cleanup happens automatically.
 pub fn run_serve_test(test_name: &str, callback: impl FnOnce(TestServeSession, RedactionMap)) {
     let _ = env_logger::try_init();
 
@@ -36,42 +45,7 @@ pub fn run_serve_test(test_name: &str, callback: impl FnOnce(TestServeSession, R
     settings.bind(move || callback(session, redactions));
 }
 
-pub trait Internable<T> {
+/// Represents a running Rojo serve session running in a temporary directory.
-    fn intern(&self, redactions: &mut RedactionMap, extra: T);
-}
-
-impl Internable<RbxId> for ReadResponse<'_> {
-    fn intern(&self, redactions: &mut RedactionMap, root_id: RbxId) {
-        redactions.intern(root_id);
-
-        let root_instance = self.instances.get(&root_id).unwrap();
-
-        for &child_id in root_instance.children.iter() {
-            self.intern(redactions, child_id);
-        }
-    }
-}
-
-pub trait InternAndRedact<T> {
-    fn intern_and_redact(&self, redactions: &mut RedactionMap, extra: T) -> serde_yaml::Value;
-}
-
-impl<I, T> InternAndRedact<T> for I
-where
-    I: Serialize + Internable<T>,
-{
-    fn intern_and_redact(&self, redactions: &mut RedactionMap, extra: T) -> serde_yaml::Value {
-        self.intern(redactions, extra);
-        redactions.redacted_yaml(self)
-    }
-}
-
-fn get_port_number() -> usize {
-    static NEXT_PORT_NUMBER: AtomicUsize = AtomicUsize::new(35103);
-
-    NEXT_PORT_NUMBER.fetch_add(1, Ordering::SeqCst)
-}
-
 pub struct TestServeSession {
     // Drop order is important here: we want the process to be killed before the
     // directory it's operating on is destroyed.
@@ -129,19 +103,28 @@ impl TestServeSession {
         &self.project_path
     }
 
+    /// Waits for the `rojo serve` server to come online with expontential
+    /// backoff.
     pub fn wait_to_come_online(&mut self) -> ServerInfoResponse {
-        loop {
+        const BASE_DURATION_MS: f32 = 30.0;
+        const EXP_BACKOFF_FACTOR: f32 = 1.3;
+        const MAX_TRIES: u32 = 5;
+
+        for i in 1..=MAX_TRIES {
             match self.rojo_process.0.try_wait() {
                 Ok(Some(status)) => panic!("Rojo process exited with status {}", status),
-                Ok(None) => { /* process is still running */ }
+                Ok(None) => { /* The process is still running, as expected */ }
                 Err(err) => panic!("Failed to wait on Rojo process: {}", err),
             }
 
             let info = match self.get_api_rojo() {
                 Ok(info) => info,
                 Err(err) => {
-                    log::debug!("Server error, retrying: {}", err);
+                    let retry_time_ms = BASE_DURATION_MS * (i as f32).powf(EXP_BACKOFF_FACTOR);
-                    std::thread::sleep(std::time::Duration::from_millis(30));
+                    let retry_time = Duration::from_millis(retry_time_ms as u64);
+
+                    log::info!("Server error, retrying in {:?}: {}", retry_time, err);
+                    thread::sleep(retry_time);
                     continue;
                 }
             };
@@ -150,6 +133,8 @@ impl TestServeSession {
 
             return info;
         }
+
+        panic!("Rojo server did not respond after {} tries.", MAX_TRIES);
     }
 
     pub fn get_api_rojo(&self) -> Result<ServerInfoResponse, reqwest::Error> {
@@ -175,3 +160,15 @@ impl TestServeSession {
         reqwest::get(&url)?.json()
     }
 }
+
+/// Probably-okay way to generate random enough port numbers for running the
+/// Rojo live server.
+///
+/// If this method ends up having problems, we should add an option for Rojo to
+/// use a random port chosen by the operating system and figure out a good way
+/// to get that port back to the test CLI.
+fn get_port_number() -> usize {
+    static NEXT_PORT_NUMBER: AtomicUsize = AtomicUsize::new(35103);
+
+    NEXT_PORT_NUMBER.fetch_add(1, Ordering::SeqCst)
+}