Merge pull request #113 from shadr/improve-tests

Check idempotence in tests

Author: NathanLovato

queries/gdscript.scm

@@ -192,7 +192,10 @@
   (comment)
   (region_start)
   (region_end)
-  (annotation)] @allow_blank_line_before)
+  (annotation)
+  (function_definition)
+  (class_definition)
+  (constructor_definition)] @allow_blank_line_before)
 
 (setget) @prepend_indent_start @append_indent_end
 (setget ":" @prepend_antispace)

src/formatter.rs

@@ -166,7 +166,7 @@ impl Formatter {
         // - must contain at least one new line character between `extends_name` and optional doc comment
         // - may contain multiple doc comment lines that starts with `##` and ends with a new line character
         let re = RegexBuilder::new(
-            r#"(?P<extends_line>^extends )(?P<extends_name>([a-zA-Z0-9]+|".*?"))\n+(?P<doc>(?:^##.*\n)*)\n*(?P<EOF>\z)?"#,
+            r#"(?P<extends_line>^extends )(?P<extends_name>([a-zA-Z0-9]+|".*?"))\n+((?P<doc>(?:^##.*\n)+)(?:\z|\n))?\n*(?P<EOF>\z)?"#,
         )
         .multi_line(true)
         .build()

src/reorder.rs

@@ -252,12 +252,13 @@ fn extract_tokens_to_reorder(
     // We need to check if the comments are contiguous with the declaration they are
     // attached to.
     let mut class_docstring_comments = Vec::new();
-    let mut found_non_comment_non_class = false;
+    let mut class_docstring_comments_rows = Vec::new();
     for (node, text) in &all_nodes {
         match node.kind() {
             "comment" => {
-                if text.trim_start().starts_with("##") && !found_non_comment_non_class {
+                if text.trim_start().starts_with("##") {
                     class_docstring_comments.push(text.clone());
+                    class_docstring_comments_rows.push(node.start_position().row);
                 }
             }
             "class_name_statement" | "extends_statement" | "annotation" => {
@@ -266,7 +267,24 @@ fn extract_tokens_to_reorder(
             // Any other element means we're past the top of the file, so we stop
             // collecting the class docstring
             _ => {
-                found_non_comment_non_class = true;
+                // if the last node of the docstring is immediately followed by the current node
+                if class_docstring_comments_rows
+                    .last()
+                    .is_some_and(|row| row + 1 == node.start_position().row)
+                {
+                    // count how many rows are belong to the statement docstring
+                    let statement_docstring_rows = class_docstring_comments_rows
+                        .iter()
+                        .rev()
+                        .zip((0..).map(|i| class_docstring_comments_rows.last().unwrap() - i))
+                        .take_while(|(actual, expected)| **actual == *expected)
+                        .count();
+                    class_docstring_comments
+                        .truncate(class_docstring_comments.len() - statement_docstring_rows);
+                    class_docstring_comments_rows
+                        .truncate(class_docstring_comments_rows.len() - statement_docstring_rows);
+                }
+                break;
             }
         }
     }
@@ -304,7 +322,7 @@ fn extract_tokens_to_reorder(
             "comment" => {
                 // We already processed class docstring comments, so we skip them here
                 // This may look inefficient but in practice it should not have much impact
-                if text.trim_start().starts_with("##") && class_docstring_comments.contains(&text) {
+                if class_docstring_comments_rows.contains(&node.start_position().row) {
                     continue;
                 } else {
                     pending_comments.push(text);

tests/expected/class_doc_comment.gd

@@ -1,31 +1,48 @@
-class_name Player
 extends CharacterBody2D
-## A brief description of the class's role and functionality.
+## Blankline after docstring
 ##
 ## The description of script
 
-class_name Player
+var a = 10
 extends CharacterBody2D
-## A brief description of the class's role and functionality.
+## Blankline before and after docstring
 ##
 ## The description of script
 
-class_name Player
+var a = 10
 extends CharacterBody2D
-## A brief description of the class's role and functionality.
+
+## Blankline before docstring
 ##
-## The description of script
+## The description of a variable
+var a = 10
+extends CharacterBody2D
 
+## No blankline around docstring
+##
+## The description of a variable
 var a = 10
-class_name Player
 extends CharacterBody2D
-## A brief description of the class's role and functionality.
+
+## Blanklink before function docstring
+##
+## The description of a variable
+func foo():
+	pass
+extends CharacterBody2D
+## Blankline between docstring and function definition
 ##
-## The description of script
+## The description of a variable
+
+func foo():
+	pass
+extends CharacterBody2D
 
 var a = 10
-class_name Player
 extends CharacterBody2D
-## A brief description of the class's role and functionality.
 
 var a = 10
+extends CharacterBody2D
+## Docstring at the end of file
+##
+## The description of script

tests/input/class_doc_comment.gd

@@ -1,27 +1,47 @@
-class_name Player extends CharacterBody2D
-
-## A brief description of the class's role and functionality.
-##
-## The description of script
-class_name Player extends CharacterBody2D
-## A brief description of the class's role and functionality.
+extends CharacterBody2D
+## Blankline after docstring
 ##
 ## The description of script
-class_name Player extends CharacterBody2D
-## A brief description of the class's role and functionality.
+
+var a = 10
+extends CharacterBody2D
+
+## Blankline before and after docstring
 ##
 ## The description of script
 
 var a = 10
-class_name Player
 extends CharacterBody2D
 
-## A brief description of the class's role and functionality.
+## Blankline before docstring
 ##
-## The description of script
+## The description of a variable
+var a = 10
+extends CharacterBody2D
+## No blankline around docstring
+##
+## The description of a variable
+var a = 10
+extends CharacterBody2D
+## Blanklink before function docstring
+##
+## The description of a variable
+func foo():
+	pass
+extends CharacterBody2D
+## Blankline between docstring and function definition
+##
+## The description of a variable
+
+func foo():
+	pass
+extends CharacterBody2D
 
 var a = 10
-class_name Player
 extends CharacterBody2D
-## A brief description of the class's role and functionality.
 var a = 10
+
+extends CharacterBody2D
+## Docstring at the end of file
+##
+## The description of script

tests/integration_tests.rs

@@ -1,5 +1,5 @@
 use gdscript_formatter::FormatterConfig;
-use gdscript_formatter::formatter::{format_gdscript, format_gdscript_with_config};
+use gdscript_formatter::formatter::format_gdscript_with_config;
 use similar::{ChangeTag, TextDiff};
 use std::fs;
 use std::path::Path;
@@ -13,13 +13,15 @@ fn make_whitespace_visible(s: &str) -> String {
         .replace('\n', "↲\n")
 }
 
-fn assert_formatted_eq(result: &str, expected: &str, file_path: &Path) {
+fn assert_formatted_eq(
+    result: &str,
+    expected: &str,
+    file_path: &Path,
+    error_context_message: &str,
+) {
     if result != expected {
-        eprintln!(
-            "\nFormatted output doesn't match expected for {}",
-            file_path.display()
-        );
-        eprintln!("Diff between expected(-) and formatted output(+):");
+        eprintln!("\n{} - {}", error_context_message, file_path.display());
+        eprintln!("Diff between expected(-) and actual output(+):");
         let diff = TextDiff::from_lines(expected, result);
         for change in diff.iter_all_changes() {
             let text = make_whitespace_visible(&change.to_string());
@@ -34,34 +36,26 @@ fn assert_formatted_eq(result: &str, expected: &str, file_path: &Path) {
         eprintln!("{:?}", expected);
         eprintln!("\nGOT (raw):");
         eprintln!("{:?}", result);
-        panic!("Assertion failed");
+        panic!("Assertion failed: {}", error_context_message);
     }
 }
 
 fn test_file(file_path: &Path) {
-    let file_name = file_path.file_name().expect("path is not a file path");
-
-    let input_path = file_path;
-    let expected_path = file_path
-        .parent()
-        .unwrap()
-        .parent()
-        .unwrap()
-        .join("expected/")
-        .join(file_name);
-
-    let input_content =
-        fs::read_to_string(&input_path).expect(&format!("Failed to read {}", input_path.display()));
-    let expected_content = fs::read_to_string(&expected_path)
-        .expect(&format!("Failed to read {}", expected_path.display()));
-
-    let result = format_gdscript(&input_content)
-        .expect(&format!("Failed to format {}", input_path.display()));
-
-    assert_formatted_eq(&result, &expected_content, &input_path);
+    test_file_with_config(file_path, &FormatterConfig::default(), true);
 }
 
 fn test_reorder_file(file_path: &Path) {
+    test_file_with_config(
+        file_path,
+        &FormatterConfig {
+            reorder_code: true,
+            ..Default::default()
+        },
+        true,
+    );
+}
+
+fn test_file_with_config(file_path: &Path, config: &FormatterConfig, check_idempotence: bool) {
     let file_name = file_path.file_name().expect("path is not a file path");
 
     let input_path = file_path;
@@ -78,14 +72,24 @@ fn test_reorder_file(file_path: &Path) {
     let expected_content = fs::read_to_string(&expected_path)
         .expect(&format!("Failed to read {}", expected_path.display()));
 
-    let result = format_gdscript_with_config(
-        &input_content,
-        &FormatterConfig {
-            reorder_code: true,
-            ..Default::default()
-        },
-    )
-    .expect(&format!("Failed to format {}", input_path.display()));
+    let result = format_gdscript_with_config(&input_content, config)
+        .expect(&format!("Failed to format {}", input_path.display()));
+
+    assert_formatted_eq(
+        &result,
+        &expected_content,
+        &input_path,
+        "First formatting output doesn't match expected",
+    );
 
-    assert_formatted_eq(&result, &expected_content, &input_path);
+    if check_idempotence {
+        let second_result = format_gdscript_with_config(&result, config)
+            .expect(&format!("Failed to format {}", input_path.display()));
+        assert_formatted_eq(
+            &second_result,
+            &result,
+            &input_path,
+            "Idempotence check failed, formatting a second time gave different results",
+        );
+    }
 }