* Re: [PATCH] Make the test output more pleasant
2021-11-11 5:53 [PATCH] Make the test output more pleasant tangmeng
@ 2021-11-17 12:18 ` Dodji Seketeli
0 siblings, 0 replies; 2+ messages in thread
From: Dodji Seketeli @ 2021-11-17 12:18 UTC (permalink / raw)
To: tangmeng; +Cc: libabigail
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
--
2.32.0
From 41625582a3bb5a4303b60aef4e3632e7f581dce9 Mon Sep 17 00:00:00 2001
From: Dodji Seketeli <dodji@redhat.com>
Date: Wed, 17 Nov 2021 12:00:52 +0100
Subject: [PATCH 2/3] test-utils: Define test status reporting functions
* tests/test-utils.h (emit_test_status_and_update_counters)
(emit_test_summary): Declare ...
* tests/test-utils.cc (emit_test_status_and_update_counters)
(emit_test_summary): ... new functions.
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
tests/test-utils.cc | 63 +++++++++++++++++++++++++++++++++++++++++++++
tests/test-utils.h | 11 +++++++-
2 files changed, 73 insertions(+), 1 deletion(-)
diff --git a/tests/test-utils.cc b/tests/test-utils.cc
index db169cc3..f317c580 100644
--- a/tests/test-utils.cc
+++ b/tests/test-utils.cc
@@ -3,6 +3,7 @@
//
// Copyright (C) 2013-2020 Red Hat, Inc.
+#include <iostream>
#include "test-utils.h"
using std::string;
@@ -40,5 +41,67 @@ get_build_dir()
return s;
}
+/// Emit test status on the standard output.
+///
+/// This function also increments passed, failed and total test
+/// numbers accordingly.
+///
+/// @param test_passed indicated if the test succeeded or not.
+///
+/// @param test_cmd the test command that was executed. If the test
+/// failed, the exact command is displayed.
+///
+/// @param passed_count the number of passed tests. This is going to
+/// be incremented if the test passes.
+///
+/// @param failed_count the number of failed tests. This is going to
+/// be incremented if the test fails.
+///
+/// @param total_count the total number of tests. This is going to be
+/// incremented.
+void
+emit_test_status_and_update_counters(bool test_passed,
+ const std::string& test_cmd,
+ unsigned& passed_count,
+ unsigned& failed_count,
+ unsigned& total_count)
+{
+ if (test_passed)
+ passed_count++;
+ else
+ {
+ std::cout << TEST_FAILURE_COLOR
+ << "Test Failed: "
+ << DEFAULT_TERMINAL_COLOR
+ << test_cmd
+ << std::endl;
+ failed_count++;
+ }
+ total_count++;
+}
+
+/// Emit the summary of the test.
+///
+/// @param total_count the total number of tests executed.
+///
+/// @param passed_count the number of tests that succeeded.
+///
+/// @param failed_count the number of tests that failed.
+void
+emit_test_summary(unsigned total_count,
+ unsigned passed_count,
+ unsigned failed_count)
+{
+ if (failed_count)
+ std::cout << TEST_FAILURE_COLOR << "FAILURE!";
+ else
+ std::cout << TEST_SUCCESS_COLOR << "SUCCESS!";
+ std::cout << DEFAULT_TERMINAL_COLOR << "\n";
+
+ std::cout << "Total number of tests executed: " << total_count
+ << " Number of tests PASSED: " << passed_count
+ << ", Number of tests FAILED: " << failed_count
+ << ".\n";
+}
}//end namespace tests
}//end namespace abigail
diff --git a/tests/test-utils.h b/tests/test-utils.h
index 9f6d1e06..e1fd17d5 100644
--- a/tests/test-utils.h
+++ b/tests/test-utils.h
@@ -23,7 +23,16 @@ namespace tests
const char* get_src_dir();
const char* get_build_dir();
-
+void
+emit_test_status_and_update_counters(bool test_passed,
+ const std::string& test_cmd,
+ unsigned& passed_count,
+ unsigned& failed_count,
+ unsigned& total_count);
+void
+emit_test_summary(unsigned total_count,
+ unsigned passed_count,
+ unsigned failed_count);
}//end namespace tests
}//end namespace abigail
#endif //__TEST_UTILS_H__
--
2.32.0
From eea4e92b2d106e1d845738d8f5fdaf12ed0e4c81 Mon Sep 17 00:00:00 2001
From: tangmeng <tangmeng@uniontech.com>
Date: Thu, 11 Nov 2021 13:53:42 +0800
Subject: [PATCH 3/3] Standardize and improve the output of several tests
This patch updates several test harnesses to make their output show
the command line of the failing tests, a brief informative summary
about the number of unit tests executed, failed and executed with
success.
These tests now used the
abigail::tests::emit_test_{summary,status_and_update_counters}
functions provided in tests/test-utils.cc.
* tests/test-abidiff-exit.cc (main): Use
abigail::tests::emit_test_{summary, status_and_update_counters}
functions to ameliorate and standardize test output.
* tests/test-alt-dwarf-file.cc (main): Likewise.
* tests/test-annotate.cc (main): Likewise.
* tests/test-diff-dwarf-abixml.cc (main): Likewise.
* tests/test-ini.cc (main): Likewise.
* tests/test-lookup-syms.cc (main): Likewise.
Signed-off-by: tangmeng <tangmeng@uniontech.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
tests/test-abidiff-exit.cc | 23 ++++++++++++++++++-----
tests/test-alt-dwarf-file.cc | 19 ++++++++++++-------
tests/test-annotate.cc | 19 ++++++++++++++-----
tests/test-diff-dwarf-abixml.cc | 19 ++++++++++++++-----
tests/test-ini.cc | 19 ++++++++++++++-----
tests/test-lookup-syms.cc | 17 +++++++++++++----
6 files changed, 85 insertions(+), 31 deletions(-)
diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
index 9ed1d6d7..15acfed9 100644
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -27,6 +27,8 @@
#include "test-utils.h"
using abigail::tools_utils::abidiff_status;
+using abigail::tests::emit_test_status_and_update_counters;
+using abigail::tests::emit_test_summary;
struct InOutSpec
{
@@ -453,9 +455,10 @@ main()
using abigail::tools_utils::split_string;
using abigail::tools_utils::abidiff_status;
- bool is_ok = true;
+ unsigned int total_count = 0, passed_count = 0, failed_count = 0;
+
string in_elfv0_path, in_elfv1_path,
- in_suppression_path, abidiff_options, abidiff, cmd,
+ in_suppression_path, abidiff_options, abidiff, cmd, diff_cmd,
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 +466,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 +533,23 @@ main()
if (abidiff_ok)
{
- cmd = "diff -u " + ref_diff_report_path
+ diff_cmd = "diff -u " + ref_diff_report_path
+ " " + out_diff_report_path;
- if (system(cmd.c_str()))
+ if (system(diff_cmd.c_str()))
is_ok = false;
}
else
is_ok = false;
+
+ emit_test_status_and_update_counters(is_ok,
+ cmd,
+ passed_count,
+ failed_count,
+ total_count);
}
- return !is_ok;
+ emit_test_summary(total_count, passed_count, failed_count);
+
+
+ return failed_count;
}
diff --git a/tests/test-alt-dwarf-file.cc b/tests/test-alt-dwarf-file.cc
index b774a1f6..315296ab 100644
--- a/tests/test-alt-dwarf-file.cc
+++ b/tests/test-alt-dwarf-file.cc
@@ -17,6 +17,8 @@
using std::cerr;
using std::string;
+using abigail::tests::emit_test_status_and_update_counters;
+using abigail::tests::emit_test_summary;
struct InOutSpec
{
@@ -70,6 +72,8 @@ main()
using abigail::tests::get_build_dir;
using abigail::tools_utils::ensure_parent_dir_created;
+ unsigned int total_count = 0, passed_count = 0, failed_count = 0;
+
bool is_ok = true;
string in_elf_path, ref_report_path, out_report_path, debug_info_dir;
string abidw, abidw_options;
@@ -101,16 +105,17 @@ main()
if (abidw_ok)
{
- cmd = "diff -u " + ref_report_path + " " + out_report_path;
- if (system(cmd.c_str()))
+ string diff_cmd = "diff -u " + ref_report_path + " " + out_report_path;
+ if (system(diff_cmd.c_str()))
is_ok &=false;
}
else
- {
- cerr << "command failed: " << cmd << "\n";
- is_ok &= false;
- }
+ is_ok &= false;
+
+ emit_test_status_and_update_counters(is_ok, cmd, passed_count,
+ failed_count, total_count);
}
- return !is_ok;
+ emit_test_summary(total_count, passed_count, failed_count);
+ return failed_count;
}
diff --git a/tests/test-annotate.cc b/tests/test-annotate.cc
index d6504dd4..bc4ce852 100644
--- a/tests/test-annotate.cc
+++ b/tests/test-annotate.cc
@@ -14,6 +14,8 @@
using std::cerr;
using std::string;
+using abigail::tests::emit_test_status_and_update_counters;
+using abigail::tests::emit_test_summary;
struct InOutSpec
{
@@ -140,7 +142,8 @@ main()
using abigail::tests::get_build_dir;
using abigail::tools_utils::ensure_parent_dir_created;
- bool is_ok = true;
+ unsigned int total_count = 0, passed_count = 0, failed_count = 0;
+
string in_elf_path, ref_report_path, out_report_path;
string abidw;
@@ -148,6 +151,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 +172,21 @@ main()
if (abidw_ok)
{
- cmd = "diff -u " + ref_report_path + " " + out_report_path;
- if (system(cmd.c_str()))
+ string diff_cmd =
+ "diff -u " + ref_report_path + " " + out_report_path;
+ if (system(diff_cmd.c_str()))
is_ok &=false;
}
else
{
- cerr << "command failed: " << cmd << "\n";
is_ok &= false;
}
+
+ emit_test_status_and_update_counters(is_ok, cmd, passed_count,
+ failed_count, total_count);
}
- return !is_ok;
+ emit_test_summary(total_count, passed_count, failed_count);
+
+ return failed_count;
}
diff --git a/tests/test-diff-dwarf-abixml.cc b/tests/test-diff-dwarf-abixml.cc
index 66ba81fa..deb7b25a 100644
--- a/tests/test-diff-dwarf-abixml.cc
+++ b/tests/test-diff-dwarf-abixml.cc
@@ -20,6 +20,8 @@ using std::string;
using std::ofstream;
using std::cerr;
using abigail::tests::get_build_dir;
+using abigail::tests::emit_test_status_and_update_counters;
+using abigail::tests::emit_test_summary;
/// Specifies where a test should get its inputs from, and where it
/// should write its output to.
@@ -57,12 +59,14 @@ main()
using abigail::tools_utils::ensure_parent_dir_created;
using abigail::tools_utils::abidiff_status;
- bool is_ok = true;
+ unsigned int total_count = 0, passed_count = 0, failed_count = 0;
+
string in_elf_path, in_abi_path,
- abidiff, cmd, ref_diff_report_path, out_diff_report_path;
+ abidiff, cmd, diff_cmd, 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 =
@@ -102,14 +106,19 @@ main()
}
if (abidiff_ok)
{
- cmd = "diff -u " + ref_diff_report_path
+ diff_cmd = "diff -u " + ref_diff_report_path
+ " " + out_diff_report_path;
- if (system(cmd.c_str()))
+ if (system(diff_cmd.c_str()))
is_ok = false;
}
else
is_ok = false;
+
+ emit_test_status_and_update_counters(is_ok, cmd, passed_count,
+ failed_count, total_count);
}
- return !is_ok;
+ emit_test_summary(total_count, passed_count, failed_count);
+
+ return failed_count;
}
diff --git a/tests/test-ini.cc b/tests/test-ini.cc
index f906e50b..5d41efbc 100644
--- a/tests/test-ini.cc
+++ b/tests/test-ini.cc
@@ -21,6 +21,8 @@
using std::string;
using std::cerr;
+using abigail::tests::emit_test_status_and_update_counters;
+using abigail::tests::emit_test_summary;
/// This is an aggregate that specifies where a test shall get its
/// input from and where it shall write its ouput to.
@@ -82,11 +84,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 total_count = 0, passed_count = 0, failed_count = 0;
+
+ string in_ini_path, in_reference_output_path, out_ini_path, cmd, diff_cmd;
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 +124,18 @@ main()
if (cmd_is_ok)
{
- cmd = "diff -u " + in_reference_output_path + " " + out_ini_path;
- if (system(cmd.c_str()))
+ diff_cmd = "diff -u " + in_reference_output_path + " " + out_ini_path;
+ if (system(diff_cmd.c_str()))
is_ok = false;
}
else
is_ok = false;
+
+ emit_test_status_and_update_counters(is_ok, cmd, passed_count,
+ failed_count, total_count);
}
- return !is_ok;
+ emit_test_summary(total_count, passed_count, failed_count);
+
+ return failed_count;
}
diff --git a/tests/test-lookup-syms.cc b/tests/test-lookup-syms.cc
index 5c63868e..096a303e 100644
--- a/tests/test-lookup-syms.cc
+++ b/tests/test-lookup-syms.cc
@@ -18,6 +18,8 @@
using std::cerr;
using std::string;
+using abigail::tests::emit_test_status_and_update_counters;
+using abigail::tests::emit_test_summary;
struct InOutSpec
{
@@ -90,12 +92,14 @@ main()
using abigail::tests::get_build_dir;
using abigail::tools_utils::ensure_parent_dir_created;
- bool is_ok = true;
+ unsigned int total_count = 0, passed_count = 0, failed_count = 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 +127,18 @@ main()
if (abisym_ok)
{
- cmd = "diff -u " + ref_report_path + " "+ out_report_path;
- if (system(cmd.c_str()))
+ string diff_cmd = "diff -u " + ref_report_path + " "+ out_report_path;
+ if (system(diff_cmd.c_str()))
is_ok = false;
}
else
is_ok = false;
+
+ emit_test_status_and_update_counters(is_ok, cmd, passed_count,
+ failed_count, total_count);
}
- return !is_ok;
+ emit_test_summary(total_count, passed_count, failed_count);
+
+ return failed_count;
}
--
2.32.0
--
Dodji
^ permalink raw reply [flat|nested] 2+ messages in thread