| Message ID | 1449704528-289297-2-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/10/2015 01:40 AM, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > tests/bios-tables-test.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c > index 6d37332..50678b5 100644 > --- a/tests/bios-tables-test.c > +++ b/tests/bios-tables-test.c > @@ -580,6 +580,13 @@ static void test_acpi_asl(test_data *data) > (gchar *)&signature, > sdt->asl_file, sdt->aml_file, > exp_sdt->asl_file, exp_sdt->aml_file); > + if (getenv("V")) { > + int ret G_GNUC_UNUSED; > + char *diff = g_strdup_printf("git diff --exit-code %s %s", > + exp_sdt->asl_file, sdt->asl_file); > + ret = system(diff) ; > + g_free(diff); > + } > } > } > g_string_free(asl, true); > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
On Thu, Dec 10, 2015 at 12:40:55AM +0100, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > tests/bios-tables-test.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c > index 6d37332..50678b5 100644 > --- a/tests/bios-tables-test.c > +++ b/tests/bios-tables-test.c > @@ -580,6 +580,13 @@ static void test_acpi_asl(test_data *data) > (gchar *)&signature, > sdt->asl_file, sdt->aml_file, > exp_sdt->asl_file, exp_sdt->aml_file); > + if (getenv("V")) { How do you test? make check V=1? > + int ret G_GNUC_UNUSED; > + char *diff = g_strdup_printf("git diff --exit-code %s %s", > + exp_sdt->asl_file, sdt->asl_file); This assumes running under git which is generally wrong e.g. people build from tarballs. git is also not a build dependency. > + ret = system(diff) ; > + g_free(diff); > + } > } > } > g_string_free(asl, true); > -- > 1.8.3.1 >
On Tue, 22 Dec 2015 17:28:42 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Dec 10, 2015 at 12:40:55AM +0100, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > tests/bios-tables-test.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c > > index 6d37332..50678b5 100644 > > --- a/tests/bios-tables-test.c > > +++ b/tests/bios-tables-test.c > > @@ -580,6 +580,13 @@ static void test_acpi_asl(test_data *data) > > (gchar *)&signature, > > sdt->asl_file, sdt->aml_file, > > exp_sdt->asl_file, exp_sdt->aml_file); > > + if (getenv("V")) { > > How do you test? make check V=1? yep make V=1 check > > > + int ret G_GNUC_UNUSED; > > + char *diff = g_strdup_printf("git diff --exit-code %s %s", > > + exp_sdt->asl_file, sdt->asl_file); > > This assumes running under git which is generally wrong e.g. > people build from tarballs. > git is also not a build dependency. usually only developers use V=1 as they care about verbose output and they probably use/have git so it doesn't have to builddep, if they don't they won't get any useful output here. my choice of 'git diff' was dictated by the fact that it produces nice colored diff of ASL while usual diff doesn't. And that helps a lot (to me) as that diff is intended for human consumption. > > > > + ret = system(diff) ; > > + g_free(diff); > > + } > > } > > } > > g_string_free(asl, true); > > -- > > 1.8.3.1 > >
On Tue, Dec 22, 2015 at 04:54:06PM +0100, Igor Mammedov wrote: > On Tue, 22 Dec 2015 17:28:42 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, Dec 10, 2015 at 12:40:55AM +0100, Igor Mammedov wrote: > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > tests/bios-tables-test.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c > > > index 6d37332..50678b5 100644 > > > --- a/tests/bios-tables-test.c > > > +++ b/tests/bios-tables-test.c > > > @@ -580,6 +580,13 @@ static void test_acpi_asl(test_data *data) > > > (gchar *)&signature, > > > sdt->asl_file, sdt->aml_file, > > > exp_sdt->asl_file, exp_sdt->aml_file); > > > + if (getenv("V")) { > > > > How do you test? make check V=1? > yep > make V=1 check > > > > > > + int ret G_GNUC_UNUSED; > > > + char *diff = g_strdup_printf("git diff --exit-code %s %s", > > > + exp_sdt->asl_file, sdt->asl_file); > > > > This assumes running under git which is generally wrong e.g. > > people build from tarballs. > > git is also not a build dependency. > usually only developers use V=1 as they care about verbose output > and they probably use/have git so it doesn't have to builddep, > if they don't they won't get any useful output here. > > my choice of 'git diff' was dictated by the fact that > it produces nice colored diff of ASL while usual diff doesn't. > And that helps a lot (to me) as that diff is intended > for human consumption. Well for one, I want make check to pass before I commit a new expected, not afterwards. Add another variable to specify which diff to use, install colordiff and use that. > > > > > > > + ret = system(diff) ; > > > + g_free(diff); > > > + } > > > } > > > } > > > g_string_free(asl, true); > > > -- > > > 1.8.3.1 > > >
On Tue, 22 Dec 2015 18:18:53 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Dec 22, 2015 at 04:54:06PM +0100, Igor Mammedov wrote: > > On Tue, 22 Dec 2015 17:28:42 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Thu, Dec 10, 2015 at 12:40:55AM +0100, Igor Mammedov wrote: > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > tests/bios-tables-test.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c > > > > index 6d37332..50678b5 100644 > > > > --- a/tests/bios-tables-test.c > > > > +++ b/tests/bios-tables-test.c > > > > @@ -580,6 +580,13 @@ static void test_acpi_asl(test_data *data) > > > > (gchar *)&signature, > > > > sdt->asl_file, sdt->aml_file, > > > > exp_sdt->asl_file, exp_sdt->aml_file); > > > > + if (getenv("V")) { > > > > > > How do you test? make check V=1? > > yep > > make V=1 check > > > > > > > > > + int ret G_GNUC_UNUSED; > > > > + char *diff = g_strdup_printf("git diff --exit-code %s %s", > > > > + exp_sdt->asl_file, sdt->asl_file); > > > > > > This assumes running under git which is generally wrong e.g. > > > people build from tarballs. > > > git is also not a build dependency. > > usually only developers use V=1 as they care about verbose output > > and they probably use/have git so it doesn't have to builddep, > > if they don't they won't get any useful output here. > > > > my choice of 'git diff' was dictated by the fact that > > it produces nice colored diff of ASL while usual diff doesn't. > > And that helps a lot (to me) as that diff is intended > > for human consumption. > > Well for one, I want make check to pass before > I commit a new expected, not afterwards. 'make check' and even 'make V=1 check' passes fine the point of this patch is to show user difference if there is any and one doesn't need to commit new expected if there aren't any changes (i.e. make check passes without diff/warning) > Add another variable to specify which diff to use, > install colordiff and use that. adding variable could work however it reduces usability as user will have to be aware of it and has to set it, I'll probably forget it and will have to dig it out every time it's needed. maybe if V=1 try 1. 'git diff' first if not 2. fallback to 'diff' if not 3. print warning to install git or diff > > > > > > > > > > > + ret = system(diff) ; > > > > + g_free(diff); > > > > + } > > > > } > > > > } > > > > g_string_free(asl, true); > > > > -- > > > > 1.8.3.1 > > > >
On Tue, Dec 22, 2015 at 04:54:06PM +0100, Igor Mammedov wrote: > On Tue, 22 Dec 2015 17:28:42 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, Dec 10, 2015 at 12:40:55AM +0100, Igor Mammedov wrote: > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > tests/bios-tables-test.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c > > > index 6d37332..50678b5 100644 > > > --- a/tests/bios-tables-test.c > > > +++ b/tests/bios-tables-test.c > > > @@ -580,6 +580,13 @@ static void test_acpi_asl(test_data *data) > > > (gchar *)&signature, > > > sdt->asl_file, sdt->aml_file, > > > exp_sdt->asl_file, exp_sdt->aml_file); > > > + if (getenv("V")) { > > > > How do you test? make check V=1? > yep > make V=1 check > > > > > > + int ret G_GNUC_UNUSED; > > > + char *diff = g_strdup_printf("git diff --exit-code %s %s", > > > + exp_sdt->asl_file, sdt->asl_file); > > > > This assumes running under git which is generally wrong e.g. > > people build from tarballs. > > git is also not a build dependency. > usually only developers use V=1 as they care about verbose output > and they probably use/have git so it doesn't have to builddep, > if they don't they won't get any useful output here. > > my choice of 'git diff' was dictated by the fact that > it produces nice colored diff of ASL while usual diff doesn't. > And that helps a lot (to me) as that diff is intended > for human consumption. I just do vimdiff on the two files it prints. Not sure all this is worth it. > > > > > > > + ret = system(diff) ; > > > + g_free(diff); > > > + } > > > } > > > } > > > g_string_free(asl, true); > > > -- > > > 1.8.3.1 > > >
On Tue, 22 Dec 2015 18:59:30 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Dec 22, 2015 at 04:54:06PM +0100, Igor Mammedov wrote: > > On Tue, 22 Dec 2015 17:28:42 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Thu, Dec 10, 2015 at 12:40:55AM +0100, Igor Mammedov wrote: > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > tests/bios-tables-test.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c > > > > index 6d37332..50678b5 100644 > > > > --- a/tests/bios-tables-test.c > > > > +++ b/tests/bios-tables-test.c > > > > @@ -580,6 +580,13 @@ static void test_acpi_asl(test_data *data) > > > > (gchar *)&signature, > > > > sdt->asl_file, sdt->aml_file, > > > > exp_sdt->asl_file, exp_sdt->aml_file); > > > > + if (getenv("V")) { > > > > > > How do you test? make check V=1? > > yep > > make V=1 check > > > > > > > > > + int ret G_GNUC_UNUSED; > > > > + char *diff = g_strdup_printf("git diff --exit-code %s %s", > > > > + exp_sdt->asl_file, sdt->asl_file); > > > > > > This assumes running under git which is generally wrong e.g. > > > people build from tarballs. > > > git is also not a build dependency. > > usually only developers use V=1 as they care about verbose output > > and they probably use/have git so it doesn't have to builddep, > > if they don't they won't get any useful output here. > > > > my choice of 'git diff' was dictated by the fact that > > it produces nice colored diff of ASL while usual diff doesn't. > > And that helps a lot (to me) as that diff is intended > > for human consumption. > > I just do vimdiff on the two files it prints. > Not sure all this is worth it. yep, it's something that I was doing manually as well, but after 50th or so times it was taking too much time, hence a patch that saves my and hopefully others time. Alternative solution if V=1: 1. use variable to specify diff command 2. if variable is not defined, print warning to set it that way user will be informed about diff option when 'make V=1 check' is run and diff is not specified. > > > > > > > > > > > + ret = system(diff) ; > > > > + g_free(diff); > > > > + } > > > > } > > > > } > > > > g_string_free(asl, true); > > > > -- > > > > 1.8.3.1 > > > >
On Tue, Dec 22, 2015 at 06:22:08PM +0100, Igor Mammedov wrote: > On Tue, 22 Dec 2015 18:59:30 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Dec 22, 2015 at 04:54:06PM +0100, Igor Mammedov wrote: > > > On Tue, 22 Dec 2015 17:28:42 +0200 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Thu, Dec 10, 2015 at 12:40:55AM +0100, Igor Mammedov wrote: > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > --- > > > > > tests/bios-tables-test.c | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c > > > > > index 6d37332..50678b5 100644 > > > > > --- a/tests/bios-tables-test.c > > > > > +++ b/tests/bios-tables-test.c > > > > > @@ -580,6 +580,13 @@ static void test_acpi_asl(test_data *data) > > > > > (gchar *)&signature, > > > > > sdt->asl_file, sdt->aml_file, > > > > > exp_sdt->asl_file, exp_sdt->aml_file); > > > > > + if (getenv("V")) { > > > > > > > > How do you test? make check V=1? > > > yep > > > make V=1 check > > > > > > > > > > > > + int ret G_GNUC_UNUSED; > > > > > + char *diff = g_strdup_printf("git diff --exit-code %s %s", > > > > > + exp_sdt->asl_file, sdt->asl_file); > > > > > > > > This assumes running under git which is generally wrong e.g. > > > > people build from tarballs. > > > > git is also not a build dependency. > > > usually only developers use V=1 as they care about verbose output > > > and they probably use/have git so it doesn't have to builddep, > > > if they don't they won't get any useful output here. > > > > > > my choice of 'git diff' was dictated by the fact that > > > it produces nice colored diff of ASL while usual diff doesn't. > > > And that helps a lot (to me) as that diff is intended > > > for human consumption. > > > > I just do vimdiff on the two files it prints. > > Not sure all this is worth it. > yep, it's something that I was doing manually as well, > but after 50th or so times it was taking too much time, > hence a patch that saves my and hopefully others time. > Alternative solution if V=1: > 1. use variable to specify diff command > 2. if variable is not defined, print warning to set it > > that way user will be informed about diff option when > 'make V=1 check' is run and diff is not specified. OK. > > > > > > > > > > > > > > > + ret = system(diff) ; > > > > > + g_free(diff); > > > > > + } > > > > > } > > > > > } > > > > > g_string_free(asl, true); > > > > > -- > > > > > 1.8.3.1 > > > > >
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c index 6d37332..50678b5 100644 --- a/tests/bios-tables-test.c +++ b/tests/bios-tables-test.c @@ -580,6 +580,13 @@ static void test_acpi_asl(test_data *data) (gchar *)&signature, sdt->asl_file, sdt->aml_file, exp_sdt->asl_file, exp_sdt->aml_file); + if (getenv("V")) { + int ret G_GNUC_UNUSED; + char *diff = g_strdup_printf("git diff --exit-code %s %s", + exp_sdt->asl_file, sdt->asl_file); + ret = system(diff) ; + g_free(diff); + } } } g_string_free(asl, true);
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- tests/bios-tables-test.c | 7 +++++++ 1 file changed, 7 insertions(+)