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
@@ -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;
}
@@ -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;
}
@@ -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;
}
@@ -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;
}
@@ -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;
}
@@ -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;
}