don't add the args until the end, as a new event

mikrostew · mikrostew · commit b367410b9563 · 2022-03-17T15:35:07.000-04:00
diff --git a/crates/volta-core/src/event.rs b/crates/volta-core/src/event.rs
@@ -30,9 +30,7 @@ pub struct ErrorEnv {
 #[derive(Deserialize, Serialize, PartialEq, Debug)]
 #[serde(rename_all = "lowercase")]
 pub enum EventKind {
-    Start {
-        argv: String,
-    },
+    Start,
     End {
         exit_code: i32,
     },
@@ -44,6 +42,9 @@ pub enum EventKind {
     ToolEnd {
         exit_code: i32,
     },
+    Args {
+        argv: String,
+    },
 }
 
 impl EventKind {
@@ -101,16 +102,7 @@ impl EventLog {
     }
 
     pub fn add_event_start(&mut self, activity_kind: ActivityKind) {
-        let argv = env::args_os()
-            .enumerate()
-            .fold(String::new(), |mut result, (i, arg)| {
-                if i > 0 {
-                    result.push(' ');
-                }
-                result.push_str(&arg.to_string_lossy());
-                result
-            });
-        self.add_event(EventKind::Start { argv }, activity_kind)
+        self.add_event(EventKind::Start, activity_kind)
     }
     pub fn add_event_end(&mut self, activity_kind: ActivityKind, exit_code: ExitCode) {
         self.add_event(
@@ -133,6 +125,18 @@ impl EventLog {
             activity_kind,
         )
     }
+    pub fn add_event_args(&mut self) {
+        let argv = env::args_os()
+            .enumerate()
+            .fold(String::new(), |mut result, (i, arg)| {
+                if i > 0 {
+                    result.push(' ');
+                }
+                result.push_str(&arg.to_string_lossy());
+                result
+            });
+        self.add_event(EventKind::Args { argv }, ActivityKind::Args)
+    }
 
     fn add_event(&mut self, event_kind: EventKind, activity_kind: ActivityKind) {
         let event = event_kind.into_event(activity_kind);
@@ -167,18 +171,7 @@ pub mod tests {
         event_log.add_event_start(ActivityKind::Current);
         assert_eq!(event_log.events.len(), 1);
         assert_eq!(event_log.events[0].name, "current");
-        match event_log.events[0].event {
-            EventKind::Start { ref argv } => {
-                let re = Regex::new("volta_core").unwrap();
-                assert!(re.is_match(argv));
-            }
-            _ => {
-                panic!(
-                    "Expected EventKind::Start {{ argv }}, Got: {:?}",
-                    event_log.events[0].event
-                );
-            }
-        }
+        assert_eq!(event_log.events[0].event, EventKind::Start);
 
         event_log.add_event_end(ActivityKind::Pin, ExitCode::NetworkError);
         assert_eq!(event_log.events.len(), 2);
@@ -198,5 +191,21 @@ pub mod tests {
         assert_eq!(event_log.events.len(), 4);
         assert_eq!(event_log.events[3].name, "install");
         // not checking the error because it has too much machine-specific info
+
+        event_log.add_event_args();
+        assert_eq!(event_log.events.len(), 5);
+        assert_eq!(event_log.events[4].name, "args");
+        match event_log.events[4].event {
+            EventKind::Args { ref argv } => {
+                let re = Regex::new("volta_core").unwrap();
+                assert!(re.is_match(argv));
+            }
+            _ => {
+                panic!(
+                    "Expected EventKind::Args {{ argv }}, Got: {:?}",
+                    event_log.events[4].event
+                );
+            }
+        }
     }
 }
diff --git a/crates/volta-core/src/session.rs b/crates/volta-core/src/session.rs
@@ -36,6 +36,7 @@ pub enum ActivityKind {
     Which,
     Setup,
     Run,
+    Args,
 }
 
 impl Display for ActivityKind {
@@ -62,6 +63,7 @@ impl Display for ActivityKind {
             ActivityKind::Completions => "completions",
             ActivityKind::Which => "which",
             ActivityKind::Run => "run",
+            ActivityKind::Args => "args",
         };
         f.write_str(s)
     }
@@ -158,12 +160,14 @@ impl Session {
         }
     }
 
-    pub fn exit(self, code: ExitCode) -> ! {
+    pub fn exit(mut self, code: ExitCode) -> ! {
+        self.event_log.add_event_args();
         self.publish_to_event_log();
         code.exit();
     }
 
-    pub fn exit_tool(self, code: i32) -> ! {
+    pub fn exit_tool(mut self, code: i32) -> ! {
+        self.event_log.add_event_args();
         self.publish_to_event_log();
         exit(code);
     }
diff --git a/tests/acceptance/hooks.rs b/tests/acceptance/hooks.rs
@@ -1,7 +1,9 @@
 use std::path::PathBuf;
 use std::{thread, time};
 
-use crate::support::events_helpers::{assert_events, match_end, match_error, match_start};
+use crate::support::events_helpers::{
+    assert_events, match_args, match_end, match_error, match_start,
+};
 use crate::support::sandbox::sandbox;
 use hamcrest2::assert_that;
 use hamcrest2::prelude::*;
@@ -155,16 +157,14 @@ fn redirects_download() {
     assert_events(
         &s,
         vec![
-            (
-                "volta",
-                match_start(format!("{} install node@1.2.3", VOLTA_BINARY).as_str()),
-            ),
-            (
-                "install",
-                match_start(format!("{} install node@1.2.3", VOLTA_BINARY).as_str()),
-            ),
+            ("volta", match_start()),
+            ("install", match_start()),
             ("volta", match_error(5, "Could not download node")),
             ("volta", match_end(5)),
+            (
+                "args",
+                match_args(format!("{} install node@1.2.3", VOLTA_BINARY).as_str()),
+            ),
         ],
     );
 }
@@ -192,16 +192,14 @@ fn merges_project_and_default_hooks() {
     assert_events(
         &s,
         vec![
-            (
-                "volta",
-                match_start(format!("{} install yarn@3.2.1", VOLTA_BINARY).as_str()),
-            ),
-            (
-                "install",
-                match_start(format!("{} install yarn@3.2.1", VOLTA_BINARY).as_str()),
-            ),
+            ("volta", match_start()),
+            ("install", match_start()),
             ("volta", match_error(5, "Could not download yarn")),
             ("volta", match_end(5)),
+            (
+                "args",
+                match_args(format!("{} install yarn@3.2.1", VOLTA_BINARY).as_str()),
+            ),
         ],
     );
 
@@ -217,16 +215,14 @@ fn merges_project_and_default_hooks() {
     assert_events(
         &s,
         vec![
-            (
-                "volta",
-                match_start(format!("{} install node@10.12.1", VOLTA_BINARY).as_str()),
-            ),
-            (
-                "install",
-                match_start(format!("{} install node@10.12.1", VOLTA_BINARY).as_str()),
-            ),
+            ("volta", match_start()),
+            ("install", match_start()),
             ("volta", match_error(5, "Could not download node")),
             ("volta", match_end(5)),
+            (
+                "args",
+                match_args(format!("{} install node@10.12.1", VOLTA_BINARY).as_str()),
+            ),
         ],
     );
 }
@@ -261,16 +257,14 @@ fn merges_workspace_hooks() {
     assert_events(
         &s,
         vec![
-            (
-                "volta",
-                match_start(format!("{} pin yarn@3.1.4", VOLTA_BINARY).as_str()),
-            ),
-            (
-                "pin",
-                match_start(format!("{} pin yarn@3.1.4", VOLTA_BINARY).as_str()),
-            ),
+            ("volta", match_start()),
+            ("pin", match_start()),
             ("volta", match_error(5, "Could not download yarn")),
             ("volta", match_end(5)),
+            (
+                "args",
+                match_args(format!("{} pin yarn@3.1.4", VOLTA_BINARY).as_str()),
+            ),
         ],
     );
 
@@ -286,16 +280,14 @@ fn merges_workspace_hooks() {
     assert_events(
         &s,
         vec![
-            (
-                "volta",
-                match_start(format!("{} pin npm@5.6.7", VOLTA_BINARY).as_str()),
-            ),
-            (
-                "pin",
-                match_start(format!("{} pin npm@5.6.7", VOLTA_BINARY).as_str()),
-            ),
+            ("volta", match_start()),
+            ("pin", match_start()),
             ("volta", match_error(5, "Could not download npm")),
             ("volta", match_end(5)),
+            (
+                "args",
+                match_args(format!("{} pin npm@5.6.7", VOLTA_BINARY).as_str()),
+            ),
         ],
     );
 
diff --git a/tests/acceptance/merged_platform.rs b/tests/acceptance/merged_platform.rs
@@ -1,6 +1,6 @@
 use std::{thread, time};
 
-use crate::support::events_helpers::{assert_events, match_start, match_tool_end};
+use crate::support::events_helpers::{assert_events, match_args, match_start, match_tool_end};
 use crate::support::sandbox::{sandbox, DistroMetadata, NodeFixture, NpmFixture, YarnFixture};
 use hamcrest2::assert_that;
 use hamcrest2::prelude::*;
@@ -262,12 +262,13 @@ fn uses_project_yarn_if_available() {
     assert_events(
         &s,
         vec![
+            ("tool", match_start()),
+            ("yarn", match_start()),
+            ("tool", match_tool_end(0)),
             (
-                "tool",
-                match_start(format!("{} --version", YARN_SHIM).as_str()),
+                "args",
+                match_args(format!("{} --version", YARN_SHIM).as_str()),
             ),
-            ("yarn", match_start("--version")),
-            ("tool", match_tool_end(0)),
         ],
     );
 }
diff --git a/tests/acceptance/support/events_helpers.rs b/tests/acceptance/support/events_helpers.rs
@@ -7,14 +7,15 @@ use hamcrest2::prelude::*;
 use volta_core::event::{Event, EventKind};
 
 pub enum EventKindMatcher<'a> {
-    Start { argv: &'a str },
+    Start,
     End { exit_code: i32 },
     Error { exit_code: i32, error: &'a str },
     ToolEnd { exit_code: i32 },
+    Args { argv: &'a str },
 }
 
-pub fn match_start(argv: &str) -> EventKindMatcher {
-    EventKindMatcher::Start { argv }
+pub fn match_start() -> EventKindMatcher<'static> {
+    EventKindMatcher::Start
 }
 
 pub fn match_error(exit_code: i32, error: &str) -> EventKindMatcher {
@@ -29,6 +30,10 @@ pub fn match_tool_end(exit_code: i32) -> EventKindMatcher<'static> {
     EventKindMatcher::ToolEnd { exit_code }
 }
 
+pub fn match_args(argv: &str) -> EventKindMatcher {
+    EventKindMatcher::Args { argv }
+}
+
 pub fn assert_events(sandbox: &Sandbox, matchers: Vec<(&str, EventKindMatcher)>) {
     let events_path = sandbox.root().join("events.json");
     assert_that!(&events_path, file_exists());
@@ -39,20 +44,10 @@ pub fn assert_events(sandbox: &Sandbox, matchers: Vec<(&str, EventKindMatcher)>)
     assert_that!(events.len(), eq(matchers.len()));
 
     for (i, matcher) in matchers.iter().enumerate() {
-        //println!("Element at position {}: {:?}", i, matcher);
         assert_that!(&events[i].name, eq(matcher.0));
         match matcher.1 {
-            EventKindMatcher::Start {
-                argv: expected_argv,
-            } => {
-                if let EventKind::Start { argv } = &events[i].event {
-                    assert_that!(argv.clone(), matches_regex(expected_argv));
-                } else {
-                    panic!(
-                        "Expected: Start {{ argv: {} }}, Got: {:?}",
-                        expected_argv, events[i].event
-                    );
-                }
+            EventKindMatcher::Start => {
+                assert_that!(&events[i].event, eq(&EventKind::Start));
             }
             EventKindMatcher::End {
                 exit_code: expected_exit_code,
@@ -95,6 +90,18 @@ pub fn assert_events(sandbox: &Sandbox, matchers: Vec<(&str, EventKindMatcher)>)
                     );
                 }
             }
+            EventKindMatcher::Args {
+                argv: expected_argv,
+            } => {
+                if let EventKind::Args { argv } = &events[i].event {
+                    assert_that!(argv.clone(), matches_regex(expected_argv));
+                } else {
+                    panic!(
+                        "Expected: Args {{ argv: {} }}, Got: {:?}",
+                        expected_argv, events[i].event
+                    );
+                }
+            }
         }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -36,6 +36,7 @@ pub enum ActivityKind {`
`36`	`36`	`Which,`
`37`	`37`	`Setup,`
`38`	`38`	`Run,`
	`39`	`+ Args,`
`39`	`40`	`}`
`40`	`41`
`41`	`42`	`impl Display for ActivityKind {`
`@@ -62,6 +63,7 @@ impl Display for ActivityKind {`
`62`	`63`	`ActivityKind::Completions => "completions",`
`63`	`64`	`ActivityKind::Which => "which",`
`64`	`65`	`ActivityKind::Run => "run",`
	`66`	`+ ActivityKind::Args => "args",`
`65`	`67`	`};`
`66`	`68`	`f.write_str(s)`
`67`	`69`	`}`
`@@ -158,12 +160,14 @@ impl Session {`
`158`	`160`	`}`
`159`	`161`	`}`
`160`	`162`
`161`		`- pub fn exit(self, code: ExitCode) -> ! {`
	`163`	`+ pub fn exit(mut self, code: ExitCode) -> ! {`
	`164`	`+ self.event_log.add_event_args();`
`162`	`165`	`self.publish_to_event_log();`
`163`	`166`	`code.exit();`
`164`	`167`	`}`
`165`	`168`
`166`		`- pub fn exit_tool(self, code: i32) -> ! {`
	`169`	`+ pub fn exit_tool(mut self, code: i32) -> ! {`
	`170`	`+ self.event_log.add_event_args();`
`167`	`171`	`self.publish_to_event_log();`
`168`	`172`	`exit(code);`
`169`	`173`	`}`