begin to implement detecting and migrating conditions to ui_test

asquared31415 · asquared31415 · commit 4db5b3e49de6 · 2023-08-15T03:11:30.000Z
diff --git a/src/tools/compiletest/src/header/cfg.rs b/src/tools/compiletest/src/header/cfg.rs
@@ -42,56 +42,38 @@ pub(super) fn handle_only(config: &Config, comment: TestComment<'_>) -> IgnoreDe
 
 /// Parses a name-value directive which contains config-specific information, e.g., `ignore-x86`
 /// or `normalize-stderr-32bit`.
-pub(super) fn parse_cfg_name_directive<'line>(
+pub(super) fn parse_cfg_name_directive(
     config: &Config,
-    comment: &'line TestComment<'line>,
+    comment: &TestComment<'_>,
     prefix: &str,
-) -> MatchOutcome<'line> {
-    match comment.comment() {
-        CommentKind::Compiletest(line) => {
-            parse_cfg_name_directive_compiletest(config, line, prefix)
-        }
-        CommentKind::UiTest(line) => parse_cfg_name_directive_ui_test(config, line, prefix),
-    }
-}
-
-fn directive_name_for_line<'line, 'p>(
-    line: &'line str,
-    prefix: &'p str,
-) -> Option<(&'line str, Option<&'line str>)> {
-    // Directives start with a specified prefix, and are immediately followed by a '-'.
-    let expected_start = format!("{}-", prefix);
-    let after_prefix = if line.starts_with(expected_start.as_str()) {
-        &line[expected_start.len()..]
-    } else {
-        return None;
+) -> MatchOutcome {
+    let comment_kind = comment.comment();
+    let Some((name, comment)) = comment_kind.parse_name_comment().and_then(|(name, comment)| {
+        name.strip_prefix(format!("{}-", prefix).as_str()).map(|stripped| (stripped, comment))
+    }) else {
+        return MatchOutcome::NotADirective;
     };
+    let comment = comment.map(|c| c.trim().trim_start_matches('-').trim().to_owned());
 
-    // If there is a ':' or a ' ' (space), split the name off, and consider the rest of the line to
-    // be a "comment" that is ignored.
-    let (name, comment) = after_prefix
-        .split_once(&[':', ' '])
-        .map(|(l, c)| (l.trim(), Some(c)))
-        .unwrap_or((after_prefix, None));
-
-    // Some of the matchers might be "" depending on what the target information is. To avoid
-    // problems we outright reject empty directives.
-    if name == "" { None } else { Some((name, comment)) }
+    match comment_kind {
+        CommentKind::Compiletest(_) => {
+            parse_cfg_name_directive_compiletest(config, name, comment, prefix)
+        }
+        CommentKind::UiTest(_) => parse_cfg_name_directive_ui_test(config, name, comment),
+    }
 }
 
-fn parse_cfg_name_directive_ui_test<'line>(
+fn parse_cfg_name_directive_ui_test(
     config: &Config,
-    line: &'line str,
-    prefix: &str,
-) -> MatchOutcome<'line> {
-    let Some((name, comment)) = directive_name_for_line(line, prefix) else {
-        return MatchOutcome::NotADirective;
-    };
-    let comment = comment.map(|c| c.trim().trim_start_matches('-').trim());
-
+    name: &str,
+    comment: Option<String>,
+) -> MatchOutcome {
     let target_cfg = config.target_cfg();
 
-    if name == "on-host" {
+    // Parsing copied from ui_test: https://github.com/oli-obk/ui_test/blob/a18ef37bf3dcccf5a1a631eddd55759fe0b89617/src/parser.rs#L187
+    if name == "test" {
+        MatchOutcome::Match { message: String::from("always"), comment }
+    } else if name == "on-host" {
         unimplemented!("idk what to do about this yet")
     } else if let Some(bits) = name.strip_suffix("bit") {
         let Ok(bits) = bits.parse::<u32>() else {
@@ -126,16 +108,12 @@ fn parse_cfg_name_directive_ui_test<'line>(
     }
 }
 
-fn parse_cfg_name_directive_compiletest<'a>(
+fn parse_cfg_name_directive_compiletest(
     config: &Config,
-    line: &'a str,
+    name: &str,
+    comment: Option<String>,
     prefix: &str,
-) -> MatchOutcome<'a> {
-    let Some((name, comment)) = directive_name_for_line(line, prefix) else {
-        return MatchOutcome::NotADirective;
-    };
-    let comment = comment.map(|c| c.trim().trim_start_matches('-').trim());
-
+) -> MatchOutcome {
     macro_rules! condition {
         (
             name: $name:expr,
@@ -315,11 +293,11 @@ fn parse_cfg_name_directive_compiletest<'a>(
 }
 
 #[derive(Clone, PartialEq, Debug)]
-pub(super) enum MatchOutcome<'a> {
+pub(super) enum MatchOutcome {
     /// No match.
-    NoMatch { message: String, comment: Option<&'a str> },
+    NoMatch { message: String, comment: Option<String> },
     /// Match.
-    Match { message: String, comment: Option<&'a str> },
+    Match { message: String, comment: Option<String> },
     /// The directive was invalid.
     Invalid,
     /// The directive is handled by other parts of our tooling.
diff --git a/src/tools/test_common/src/lib.rs b/src/tools/test_common/src/lib.rs
@@ -13,6 +13,7 @@ pub struct TestComment<'line> {
 }
 
 impl<'line> TestComment<'line> {
+    #[must_use]
     pub const fn new(
         line: &'line str,
         revision: Option<&'line str>,
@@ -22,23 +23,28 @@ impl<'line> TestComment<'line> {
         Self { revision, comment, line_num, full_line: line }
     }
 
+    #[must_use]
     pub const fn revision(&self) -> Option<&str> {
         self.revision
     }
 
-    pub const fn comment(&self) -> CommentKind<'_> {
+    #[must_use]
+    pub const fn comment(&'line self) -> CommentKind<'line> {
         self.comment
     }
 
+    #[must_use]
     pub const fn line_num(&self) -> usize {
         self.line_num
     }
 
+    #[must_use]
     pub const fn comment_str(&self) -> &str {
         self.comment.line()
     }
 
     /// The full line that contains the comment. You almost never want this.
+    #[must_use]
     pub const fn full_line(&self) -> &str {
         self.full_line
     }
@@ -54,12 +60,27 @@ pub enum CommentKind<'line> {
     UiTest(&'line str),
 }
 
-impl CommentKind<'_> {
-    pub const fn line(&self) -> &str {
-        match self {
+impl<'line> CommentKind<'line> {
+    #[must_use]
+    pub const fn line(&'line self) -> &'line str {
+        match *self {
             CommentKind::Compiletest(line) | CommentKind::UiTest(line) => line,
         }
     }
+
+    /// If the comment is of the form `name:extra` or `name extra`, returns the name and returns
+    /// the rest of the line as a "comment", if it exists.
+    #[must_use]
+    pub fn parse_name_comment(&'line self) -> Option<(&'line str, Option<&'line str>)> {
+        let line = self.line();
+        // If there is a ':' or a ' ' (space), split the name off, and consider the rest of the
+        // line to be a "comment" that is ignored.
+        let (name, comment) =
+            line.split_once(&[':', ' ']).map(|(l, c)| (l.trim(), Some(c))).unwrap_or((line, None));
+
+        // Reject empty names
+        if name == "" { None } else { Some((name, comment)) }
+    }
 }
 
 /// Iterates over the test header for the given `testfile`, and calls a closure
diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs
@@ -137,8 +137,8 @@ pub fn check(path: &Path, bad: &mut bool) {
             }
             "rs" => {
                 // FIXME (ui_test): make this configurable somehow
-                // let mode = HeaderCheckMode::Error;
-                let mode = HeaderCheckMode::Fix;
+                let mode = HeaderCheckMode::Error;
+                // let mode = HeaderCheckMode::Fix;
                 check_ui_test_headers(bad, file_path, mode);
             }
             _ => {}
@@ -224,23 +224,25 @@ fn fix_header_errors(file_path: &Path, bad_lines: Vec<HeaderAction>) -> io::Resu
                         let mut new_line =
                             replace_compiletest_comment(header_action.line()).unwrap();
                         // This is always a directive that contains the compiletest name.
-                        let name_start = new_line.find(compiletest_name).unwrap();
+                        let name_start = new_line.find(compiletest_name.as_str()).unwrap();
                         new_line.replace_range(
                             name_start..(name_start + compiletest_name.len()),
-                            ui_test_name,
+                            ui_test_name.as_str(),
                         );
                         new_line
                     }
                     LineAction::UseUITestName { compiletest_name, ui_test_name } => {
                         // This is always a directive that contains the compiletest name.
-                        let name_start = header_action.line().find(compiletest_name).unwrap();
+                        let name_start =
+                            header_action.line().find(compiletest_name.as_str()).unwrap();
                         let mut new_line = header_action.line().to_string();
                         new_line.replace_range(
                             name_start..(name_start + compiletest_name.len()),
-                            ui_test_name,
+                            ui_test_name.as_str(),
                         );
                         new_line
                     }
+                    LineAction::Error { message } => todo!(),
                 },
             )
         })
diff --git a/src/tools/tidy/src/ui_tests/ui_test_headers.rs b/src/tools/tidy/src/ui_tests/ui_test_headers.rs

Original file line number	Diff line number	Diff line change
`@@ -13,6 +13,7 @@ pub struct TestComment<'line> {`
`13`	`13`	`}`
`14`	`14`
`15`	`15`	`impl<'line> TestComment<'line> {`
	`16`	`+ #[must_use]`
`16`	`17`	`pub const fn new(`
`17`	`18`	`line: &'line str,`
`18`	`19`	`revision: Option<&'line str>,`
`@@ -22,23 +23,28 @@ impl<'line> TestComment<'line> {`
`22`	`23`	`Self { revision, comment, line_num, full_line: line }`
`23`	`24`	`}`
`24`	`25`
	`26`	`+ #[must_use]`
`25`	`27`	`pub const fn revision(&self) -> Option<&str> {`
`26`	`28`	`self.revision`
`27`	`29`	`}`
`28`	`30`
`29`		`- pub const fn comment(&self) -> CommentKind<'_> {`
	`31`	`+ #[must_use]`
	`32`	`+ pub const fn comment(&'line self) -> CommentKind<'line> {`
`30`	`33`	`self.comment`
`31`	`34`	`}`
`32`	`35`
	`36`	`+ #[must_use]`
`33`	`37`	`pub const fn line_num(&self) -> usize {`
`34`	`38`	`self.line_num`
`35`	`39`	`}`
`36`	`40`
	`41`	`+ #[must_use]`
`37`	`42`	`pub const fn comment_str(&self) -> &str {`
`38`	`43`	`self.comment.line()`
`39`	`44`	`}`
`40`	`45`
`41`	`46`	`/// The full line that contains the comment. You almost never want this.`
	`47`	`+ #[must_use]`
`42`	`48`	`pub const fn full_line(&self) -> &str {`
`43`	`49`	`self.full_line`
`44`	`50`	`}`
`@@ -54,12 +60,27 @@ pub enum CommentKind<'line> {`
`54`	`60`	`UiTest(&'line str),`
`55`	`61`	`}`
`56`	`62`
`57`		`-impl CommentKind<'_> {`
`58`		`- pub const fn line(&self) -> &str {`
`59`		`- match self {`
	`63`	`+impl<'line> CommentKind<'line> {`
	`64`	`+ #[must_use]`
	`65`	`+ pub const fn line(&'line self) -> &'line str {`
	`66`	`+ match *self {`
`60`	`67`	`CommentKind::Compiletest(line) \| CommentKind::UiTest(line) => line,`
`61`	`68`	`}`
`62`	`69`	`}`
	`70`	`+`
	`71`	+ /// If the comment is of the form `name:extra` or `name extra`, returns the name and returns
	`72`	`+ /// the rest of the line as a "comment", if it exists.`
	`73`	`+ #[must_use]`
	`74`	`+ pub fn parse_name_comment(&'line self) -> Option<(&'line str, Option<&'line str>)> {`
	`75`	`+ let line = self.line();`
	`76`	`+ // If there is a ':' or a ' ' (space), split the name off, and consider the rest of the`
	`77`	`+ // line to be a "comment" that is ignored.`
	`78`	`+ let (name, comment) =`
	`79`	`+ line.split_once(&[':', ' ']).map(\|(l, c)\| (l.trim(), Some(c))).unwrap_or((line, None));`
	`80`	`+`
	`81`	`+ // Reject empty names`
	`82`	`+ if name == "" { None } else { Some((name, comment)) }`
	`83`	`+ }`
`63`	`84`	`}`
`64`	`85`
`65`	`86`	/// Iterates over the test header for the given `testfile`, and calls a closure