diff mbox series

Make the test output more pleasant

Message ID 20211111055342.11995-1-tangmeng@uniontech.com
State New
Headers show
Series Make the test output more pleasant | expand

Commit Message

tangmeng Nov. 11, 2021, 5:53 a.m. UTC
Sync with the commit id of b7ba0fe8f5976251cf38f9abf249acdacdcf626b.
When testing, the following problems were found:
1. command tested multiple scenarios, but the last result was used
as the basis for the return value of the command.
2. For multiple test scenarios, the execution results cannot be known
after the test, which is easy to cause confusion.

        * tests/test-abidiff-exit.cc (main): make test output more pleasant.
        * tests/test-alt-dwarf-file.cc (main): make test output more pleasant.
        * tests/test-annotate.cc (main): make test output more pleasant.
        * tests/test-diff-dwarf-abixml.cc (main): make test output more pleasant.
        * tests/test-ini.cc (main): make test output more pleasant.
        * tests/test-lookup-syms.cc (main): make test output more pleasant.

Signed-off-by: tangmeng <tangmeng@uniontech.com>
---
 tests/test-abidiff-exit.cc      | 38 ++++++++++++++++++++++++++++-----
 tests/test-alt-dwarf-file.cc    | 35 ++++++++++++++++++++++++++----
 tests/test-annotate.cc          | 37 +++++++++++++++++++++++++++-----
 tests/test-diff-dwarf-abixml.cc | 38 ++++++++++++++++++++++++++++-----
 tests/test-ini.cc               | 38 ++++++++++++++++++++++++++++-----
 tests/test-lookup-syms.cc       | 36 +++++++++++++++++++++++++++----
 6 files changed, 194 insertions(+), 28 deletions(-)

Comments

Dodji Seketeli Nov. 17, 2021, 12:18 p.m. UTC | #1
Hello,

tangmeng <tangmeng@uniontech.com> a écrit:

Thank you for the patch!

I see that you have a real interest in ameliorating the output of the
tests.  Thank you for that!

As I am seeing more of these patches coming from you, I looked at them a
little bit deeper and came up with several observations.  I went ahead
and made various changes that complement yours.

Please find my comments below.

[...]


>         * tests/test-abidiff-exit.cc (main): make test output more pleasant.
>         * tests/test-alt-dwarf-file.cc (main): make test output more pleasant.
>         * tests/test-annotate.cc (main): make test output more pleasant.
>         * tests/test-diff-dwarf-abixml.cc (main): make test output more pleasant.
>         * tests/test-ini.cc (main): make test output more pleasant.
>         * tests/test-lookup-syms.cc (main): make test output more pleasant.

Here, the space before the '*' must be a <tab>, as explained in the
COMMIT-LOG-GUIDELINES file that is in the source tree.  You can browse
it at
https://sourceware.org/git/?p=libabigail.git;a=blob_plain;f=COMMIT-LOG-GUIDELINES;hb=HEAD.


> diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc

[...]


> +        if (is_ok)
> +          {
> +            cout << BRIGHT_YELLOW_COLOR
> +                 << "Test Passed: "
> +                 << DEFAULT_TERMINAL_COLOR
> +                 << cmd
> +                 << std::endl;

Emitting the command line of every single test is way too verbose in my
opinion.  By default, we want to have minimum information on the screen.
When something goes wrong however, we want more details so that we can
debug/investigate the issue ...

[...]

> +        else
> +          {
> +            cout << BRIGHT_RED_COLOR
> +                 << "Test Failed: "
> +                 << DEFAULT_TERMINAL_COLOR
> +                 << cmd
> +                 << std::endl;
> +            cnt_failed++;

... so here, it's cool to have the command line of the test that is
failing so that we can just run it in the debugger and investigate what
is going wrong.

> +          }
> +        cnt_total++;
>        }

This pattern has been repeated over and over in several tests.  I have
factorized it out into a function named
abigail::tests::emit_test_status_and_update_counters.  This function
does not emit the command line of the tests that succeed.  Rather, it
emits the command line of the test that fails.

You'll see below the patch where I define that function.

[...]

> +    cout << "Summary: " << cnt_total << " tested!"
> +         << " Test Passed: " << cnt_passed
> +         << ", Test Failed: " << cnt_failed
> +         << ".\n";

This pattern has also been repeated over and over in several tests of
this patch so I have factorized it out into a function named
abigail::tests::emit_test_summary.

You'll see below the patch where I define that function.

[...]

> diff --git a/tests/test-alt-dwarf-file.cc b/tests/test-alt-dwarf-file.cc

[...]

> diff --git a/tests/test-annotate.cc b/tests/test-annotate.cc

[...]

> diff --git a/tests/test-diff-dwarf-abixml.cc b/tests/test-diff-dwarf-abixml.cc

[...]

> diff --git a/tests/test-ini.cc b/tests/test-ini.cc

[...]

> diff --git a/tests/test-lookup-syms.cc b/tests/test-lookup-syms.cc

[...]

I have update all these files to make them use the two new functions I
have defined.

Please find below the three patches that I have applied to master,
complementing your work.  Your patch is the last one.  I have amended it
to make it use the first two patches and re-worded some comments and
variable names.

I guess future changes to test output should be based on this framework
being put in place now.

Thanks again for your work!

From c9e74e49d68eecf004be7f52277b90a4ddcf764d Mon Sep 17 00:00:00 2001
From: Dodji Seketeli <dodji@redhat.com>
Date: Wed, 17 Nov 2021 11:58:31 +0100
Subject: [PATCH 1/3] test-utils: Define colors for test status messages

This patch defines pre-processor macros for the colors used to emit
test SUCCESS/FAILURE status.  These are going to be used by the code,
onward.

	* tests/test-utils.h (TEST_FAILURE_COLOR, TEST_SUCCESS_COLOR):
	Define macros.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 tests/test-utils.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/test-utils.h b/tests/test-utils.h
index 5596edc6..9f6d1e06 100644
--- a/tests/test-utils.h
+++ b/tests/test-utils.h
@@ -13,6 +13,9 @@
 #define BRIGHT_RED_COLOR "\e[1;31m"
 #define DEFAULT_TERMINAL_COLOR "\033[0m"
 
+#define TEST_FAILURE_COLOR BRIGHT_RED_COLOR
+#define TEST_SUCCESS_COLOR BRIGHT_YELLOW_COLOR
+
 namespace abigail
 {
 namespace tests
diff mbox series

Patch

diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
index 9ed1d6d7..e9af39e5 100644
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -27,6 +27,7 @@ 
 #include "test-utils.h"
 
 using abigail::tools_utils::abidiff_status;
+using std::cout;
 
 struct InOutSpec
 {
@@ -453,9 +454,10 @@  main()
   using abigail::tools_utils::split_string;
   using abigail::tools_utils::abidiff_status;
 
-  bool is_ok = true;
+  unsigned int cnt_total = 0, cnt_passed = 0, cnt_failed = 0;
+
   string in_elfv0_path, in_elfv1_path,
-    in_suppression_path, abidiff_options, abidiff, cmd,
+    in_suppression_path, abidiff_options, abidiff, cmd, diffcmd,
     ref_diff_report_path, out_diff_report_path;
   vector<string> in_elfv0_headers_dirs, in_elfv1_headers_dirs;
   string source_dir_prefix = string(get_src_dir()) + "/tests/";
@@ -463,6 +465,7 @@  main()
 
     for (InOutSpec* s = in_out_specs; s->in_elfv0_path; ++s)
       {
+        bool is_ok = true;
 	in_elfv0_path = source_dir_prefix + s->in_elfv0_path;
 	in_elfv1_path = source_dir_prefix + s->in_elfv1_path;
 	split_string(s->in_elfv0_headers_dirs, ",", in_elfv0_headers_dirs);
@@ -529,14 +532,39 @@  main()
 
 	if (abidiff_ok)
 	  {
-	    cmd = "diff -u " + ref_diff_report_path
+	    diffcmd = "diff -u " + ref_diff_report_path
 	      + " " + out_diff_report_path;
-	    if (system(cmd.c_str()))
+	    if (system(diffcmd.c_str()))
 	      is_ok = false;
 	  }
 	else
 	  is_ok = false;
+
+        if (is_ok)
+          {
+            cout << BRIGHT_YELLOW_COLOR
+                 << "Test Passed: "
+                 << DEFAULT_TERMINAL_COLOR
+                 << cmd
+                 << std::endl;
+            cnt_passed++;
+          }
+        else
+          {
+            cout << BRIGHT_RED_COLOR
+                 << "Test Failed: "
+                 << DEFAULT_TERMINAL_COLOR
+                 << cmd
+                 << std::endl;
+            cnt_failed++;
+          }
+        cnt_total++;
       }
 
-    return !is_ok;
+    cout << "Summary: " << cnt_total << " tested!"
+         << " Test Passed: " << cnt_passed
+         << ", Test Failed: " << cnt_failed
+         << ".\n";
+
+    return cnt_failed;
 }
diff --git a/tests/test-alt-dwarf-file.cc b/tests/test-alt-dwarf-file.cc
index b774a1f6..c1f2fb38 100644
--- a/tests/test-alt-dwarf-file.cc
+++ b/tests/test-alt-dwarf-file.cc
@@ -16,6 +16,7 @@ 
 #include "test-utils.h"
 
 using std::cerr;
+using std::cout;
 using std::string;
 
 struct InOutSpec
@@ -70,6 +71,8 @@  main()
   using abigail::tests::get_build_dir;
   using abigail::tools_utils::ensure_parent_dir_created;
 
+  unsigned int cnt_total = 0, cnt_passed = 0, cnt_failed = 0;
+
   bool is_ok = true;
   string in_elf_path, ref_report_path, out_report_path, debug_info_dir;
   string abidw, abidw_options;
@@ -101,16 +104,40 @@  main()
 
       if (abidw_ok)
 	{
-	  cmd = "diff -u " + ref_report_path + " " + out_report_path;
-	  if (system(cmd.c_str()))
+	  string diffcmd = "diff -u " + ref_report_path + " " + out_report_path;
+	  if (system(diffcmd.c_str()))
 	    is_ok &=false;
 	}
       else
 	{
-	  cerr << "command failed: " << cmd << "\n";
 	  is_ok &= false;
 	}
+
+      if (is_ok)
+        {
+          cout << BRIGHT_YELLOW_COLOR
+               << "Test Passed: "
+               << DEFAULT_TERMINAL_COLOR
+               << cmd
+               << std::endl;
+          cnt_passed++;
+        }
+      else
+        {
+          cout << BRIGHT_RED_COLOR
+               << "Test Failed: "
+               << DEFAULT_TERMINAL_COLOR
+               << cmd
+               << std::endl;
+          cnt_failed++;
+        }
+      cnt_total++;
     }
 
-  return !is_ok;
+  cout << "Summary: " << cnt_total << " tested!"
+       << " Test Passed: " << cnt_passed
+       << ", Test Failed: " << cnt_failed
+       << ".\n";
+
+  return cnt_failed;
 }
diff --git a/tests/test-annotate.cc b/tests/test-annotate.cc
index d6504dd4..23a87799 100644
--- a/tests/test-annotate.cc
+++ b/tests/test-annotate.cc
@@ -13,6 +13,7 @@ 
 #include "test-utils.h"
 
 using std::cerr;
+using std::cout;
 using std::string;
 
 struct InOutSpec
@@ -140,7 +141,8 @@  main()
   using abigail::tests::get_build_dir;
   using abigail::tools_utils::ensure_parent_dir_created;
 
-  bool is_ok = true;
+  unsigned int cnt_total = 0, cnt_passed = 0, cnt_failed = 0;
+
   string in_elf_path, ref_report_path, out_report_path;
   string abidw;
 
@@ -148,6 +150,7 @@  main()
     "--annotate --no-corpus-path";
   for (InOutSpec* s = in_out_specs; s->in_elf_path; ++s)
     {
+      bool is_ok = true;
       in_elf_path = string(get_src_dir()) + "/tests/" + s->in_elf_path;
       ref_report_path = string(get_src_dir()) + "/tests/" + s->in_report_path;
       out_report_path =
@@ -168,16 +171,40 @@  main()
 
       if (abidw_ok)
 	{
-	  cmd = "diff -u " + ref_report_path + " " + out_report_path;
-	  if (system(cmd.c_str()))
+	  string diffcmd = "diff -u " + ref_report_path + " " + out_report_path;
+	  if (system(diffcmd.c_str()))
 	    is_ok &=false;
 	}
       else
 	{
-	  cerr << "command failed: " << cmd << "\n";
 	  is_ok &= false;
 	}
+
+      if (is_ok)
+        {
+          cout << BRIGHT_YELLOW_COLOR
+               << "Test Passed: "
+               << DEFAULT_TERMINAL_COLOR
+               << cmd
+               << std::endl;
+          cnt_passed++;
+        }
+      else
+        {
+          cout << BRIGHT_RED_COLOR
+               << "Test Failed: "
+               << DEFAULT_TERMINAL_COLOR
+               << cmd
+               << std::endl;
+          cnt_failed++;
+        }
+      cnt_total++;
     }
 
-  return !is_ok;
+  cout << "Summary: " << cnt_total << " tested!"
+       << " Test Passed: " << cnt_passed
+       << ", Test Failed: " << cnt_failed
+       << ".\n";
+
+  return cnt_failed;
 }
diff --git a/tests/test-diff-dwarf-abixml.cc b/tests/test-diff-dwarf-abixml.cc
index ba85415e..3bd1ce29 100644
--- a/tests/test-diff-dwarf-abixml.cc
+++ b/tests/test-diff-dwarf-abixml.cc
@@ -19,6 +19,7 @@ 
 using std::string;
 using std::ofstream;
 using std::cerr;
+using std::cout;
 using abigail::tests::get_build_dir;
 
 /// Specifies where a test should get its inputs from, and where it
@@ -57,12 +58,14 @@  main()
   using abigail::tools_utils::ensure_parent_dir_created;
   using abigail::tools_utils::abidiff_status;
 
-  bool is_ok = true;
+  unsigned int cnt_total = 0, cnt_passed = 0, cnt_failed = 0;
+
   string in_elf_path, in_abi_path,
-    abidiff, cmd, ref_diff_report_path, out_diff_report_path;
+    abidiff, cmd, diffcmd, ref_diff_report_path, out_diff_report_path;
 
   for (InOutSpec* s = in_out_specs; s->in_elf_path; ++s)
     {
+      bool is_ok = true;
       in_elf_path = string(get_src_dir()) + "/tests/" + s->in_elf_path;
       in_abi_path = string(get_src_dir()) + "/tests/"+ s->in_abi_path;
       ref_diff_report_path =
@@ -96,14 +99,39 @@  main()
 	}
       if (abidiff_ok)
 	{
-	  cmd = "diff -u " + ref_diff_report_path
+	  diffcmd = "diff -u " + ref_diff_report_path
 	    + " " + out_diff_report_path;
-	  if (system(cmd.c_str()))
+	  if (system(diffcmd.c_str()))
 	    is_ok = false;
 	}
       else
 	is_ok = false;
+
+      if (is_ok)
+        {
+          cout << BRIGHT_YELLOW_COLOR
+               << "Test Passed: "
+               << DEFAULT_TERMINAL_COLOR
+               << cmd
+               << std::endl;
+          cnt_passed++;
+        }
+      else
+        {
+          cout << BRIGHT_RED_COLOR
+               << "Test Failed: "
+               << DEFAULT_TERMINAL_COLOR
+               << cmd
+               << std::endl;
+          cnt_failed++;
+        }
+      cnt_total++;
     }
 
-  return !is_ok;
+  cout << "Summary: " << cnt_total << " tested!"
+       << " Test Passed: " << cnt_passed
+       << ", Test Failed: " << cnt_failed
+       << ".\n";
+
+  return cnt_failed;
 }
diff --git a/tests/test-ini.cc b/tests/test-ini.cc
index f906e50b..0e1c850e 100644
--- a/tests/test-ini.cc
+++ b/tests/test-ini.cc
@@ -21,6 +21,7 @@ 
 
 using std::string;
 using std::cerr;
+using std::cout;
 
 /// This is an aggregate that specifies where a test shall get its
 /// input from and where it shall write its ouput to.
@@ -82,11 +83,13 @@  main()
   using abigail::tools_utils::ensure_parent_dir_created;
   using abigail::tools_utils::abidiff_status;
 
-  bool is_ok = true;
-  string in_ini_path, in_reference_output_path, out_ini_path, cmd;
+  unsigned int cnt_total = 0, cnt_passed = 0, cnt_failed = 0;
+
+  string in_ini_path, in_reference_output_path, out_ini_path, cmd, diffcmd;
 
   for (InOutSpec *s = in_out_specs; s->in_ini_path; ++s)
     {
+      bool is_ok = true;
       in_ini_path = get_test_src_dir() + "/" + s->in_ini_path;
       in_reference_output_path =
 	get_test_src_dir() + "/" + s->in_reference_output_path;
@@ -120,13 +123,38 @@  main()
 
       if (cmd_is_ok)
 	{
-	  cmd = "diff -u " + in_reference_output_path + " " + out_ini_path;
-	  if (system(cmd.c_str()))
+	  diffcmd = "diff -u " + in_reference_output_path + " " + out_ini_path;
+	  if (system(diffcmd.c_str()))
 	    is_ok = false;
 	}
       else
 	is_ok = false;
+
+      if (is_ok)
+        {
+          cout << BRIGHT_YELLOW_COLOR
+               << "Test Passed: "
+               << DEFAULT_TERMINAL_COLOR
+               << cmd
+               << std::endl;
+          cnt_passed++;
+        }
+      else
+        {
+          cout << BRIGHT_RED_COLOR
+               << "Test Failed: "
+               << DEFAULT_TERMINAL_COLOR
+               << cmd
+               << std::endl;
+          cnt_failed++;
+        }
+      cnt_total++;
     }
 
-  return !is_ok;
+  cout << "Summary: " << cnt_total << " tested!"
+       << " Test Passed: " << cnt_passed
+       << ", Test Failed: " << cnt_failed
+       << ".\n";
+
+  return cnt_failed;
 }
diff --git a/tests/test-lookup-syms.cc b/tests/test-lookup-syms.cc
index 5c63868e..c3a55fb4 100644
--- a/tests/test-lookup-syms.cc
+++ b/tests/test-lookup-syms.cc
@@ -17,6 +17,7 @@ 
 #include "test-utils.h"
 
 using std::cerr;
+using std::cout;
 using std::string;
 
 struct InOutSpec
@@ -90,12 +91,14 @@  main()
   using abigail::tests::get_build_dir;
   using abigail::tools_utils::ensure_parent_dir_created;
 
-  bool is_ok = true;
+  unsigned int cnt_total = 0, cnt_passed = 0, cnt_failed = 0;
+
   string in_elf_path, symbol, abisym, abisym_options,
     ref_report_path, out_report_path;
 
   for (InOutSpec* s = in_out_specs; s->in_elf_path; ++s)
     {
+      bool is_ok = true;
       in_elf_path = string(get_src_dir()) + "/tests/" + s->in_elf_path;
       symbol = s->symbol;
       abisym_options = s->abisym_options;
@@ -123,13 +126,38 @@  main()
 
       if (abisym_ok)
 	{
-	  cmd = "diff -u " + ref_report_path + " "+  out_report_path;
-	  if (system(cmd.c_str()))
+	  string diffcmd = "diff -u " + ref_report_path + " "+  out_report_path;
+	  if (system(diffcmd.c_str()))
 	    is_ok = false;
 	}
       else
 	is_ok = false;
+
+      if (is_ok)
+        {
+          cout << BRIGHT_YELLOW_COLOR
+               << "Test Passed: "
+               << DEFAULT_TERMINAL_COLOR
+               << cmd
+               << std::endl;
+          cnt_passed++;
+        }
+      else
+        {
+          cout << BRIGHT_RED_COLOR
+               << "Test Failed: "
+               << DEFAULT_TERMINAL_COLOR
+               << cmd
+               << std::endl;
+          cnt_failed++;
+        }
+      cnt_total++;
     }
 
-  return !is_ok;
+  cout << "Summary: " << cnt_total << " tested!"
+       << " Test Passed: " << cnt_passed
+       << ", Test Failed: " << cnt_failed
+       << ".\n";
+
+  return cnt_failed;
 }