Tools: Test: Audio: Valgrind run related fixes #7719

singalsu · 2023-05-31T16:06:48Z

The changes to scripts/host-testbench.sh have unintentionally dropped valgrind run from test.

This patch enables valgrind for process_test.m runs and fixes issue in test run octave side function to silently ignore error about failed shell command. Normally test fail when there is no data or incorrect data, but a valgrind failure with correct test output was passed.

Valgrind output becomes visible if testbench run trace stderr redirection to file is disabled. If not done valgrind error would stop the test but the analysis would not be printed to console.

marc-hb

Should there be a Fixes commit abcdedf in the commit message?

marc-hb · 2023-05-31T16:16:09Z

tools/test/audio/test_utils/test_run.m

Because rcmd may print stuff too:

Suggested change

fprintf('Returned status %d\n', ret);

printf('%s eturned status %d\n', rcmd, ret);

Can't you combine fprintf and error into one? I hope one does not print on stdout while the other on stderr because buffering could order them randomly.

Error can't do the same as print but I can remove all print from error.

There's also quit() and exit() but the problem is that when working in interactive Octave or Matlab desktop they exit the desktop. And with Matlab due licence server the start is annoying slow so I don't want to use it.

Re-read the manual, today error in octave can do same as printf... and so does Matlab. It was an old limitation that no more bothers. Good that you asked!

ShriramShastry

Please have a look at my comments, which I've added.

Thank you very much.

ShriramShastry · 2023-06-05T03:28:49Z

tools/test/audio/test_utils/test_run.m

At line number 58, fprintf(fh, '#!/bin/sh\n', test.comp); Isn't the output format strange?

Good catch: it does look like a bug but how is it related to this PR?

Yep, I can add a fix commit to this PR. Strange that this is not causing errors.

ShriramShastry · 2023-06-05T03:36:46Z

tools/test/audio/process_test.m

At line number 101, tn = 1;, tn is not used; please consider using it or removing it.

At line number 321, min_bits = min(test.bits_in, test.bits_out);, do you intend to use minimum bits elsewhere ? Otherwise, it's unnecessary code that should be removed.

@ShriramShastry how is this related to this PR?

It has nothing to do with PR and does not rely on PR to succeed. A little correction can help greatly improve code quality, and I believe Seppo won't mind (because he wrote the majority of the code) considering the proposal.

These look like interesting suggestions but they're still off-topic here.

Hmm - sure it's off the topic, what are your thoughts if you notice improvement and believe that rectification would be beneficial without adding extra work? I am not sure if the chosen approach is OK or NOT, and for the PR, I request Seppo to consider but leave the decision to him.

Thanks for suggestions. They are better to leave to another PR later. If you like you are welcome to make a PR since you found them. Or I can do later when time, as you prefer.

I would want to propose that you incorporate it as well as as you can take over

Audio: Enhancing audio quality tests for device testing #6056

Thank you!!

Hmm - sure it's off the topic, what are your thoughts if you notice improvement and believe that rectification would be beneficial without adding extra work?

The very first thing when making slightly off-topic comments is to clearly warn that they are off-topic to save confusion and time. A simple "off-topic" prefix is enough. Try putting yourself in the shoes of maintainers who have to review a lot of PRs and switch context many times per day.

If some comments are totally off-topic then there are many other places. Email or instant messaging of course, or you can submit a throw-away, draft PR, or you can start a new github issue or discussion, maybe a github gist. You can also git blame the code and comment in the old, merged PR where the code appeared in the first place. Some people don't like the last option (I do). There are really a lot of options that don't noise and slow down unrelated PRs.

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html

kv2019i · 2023-06-05T08:49:44Z

tools/test/audio/process_test.m

Minor nit: the "Fixes commit-id" tag not quite following Linux style.

If your patch fixes a bug in a specific commit, e.g. you found an issue using ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of the SHA-1 ID, and the one line summary. Do not split the tag across multiple lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify parsing scripts. For example:: » Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed")

There are git scripts that scan branches for that specific Fixes: syntax.

The commit message is now fixed. You may want to re-review because of 2nd commit added to fix the syntax error found by Sriram. And strange that it didn't error.

The changes to scripts/host-testbench.sh and earlier to process_test.m have unintentionally dropped valgrind run from test. This patch enables valgrind for process_test.m runs and fixes issue in test run octave side function to silently ignore error about failed shell command. Normally test fail when there is no data or incorrect data, but a valgrind failure with correct test output was passed. Valgrind output becomes visible if testbench run trace stderr redirection to file is disabled. If not done valgrind error would stop the test but the analysis would not be printed to console. Fixes: 6b744bc ("host-testbench.sh: use process_test.m for 8 components") Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

The fprintf() command prints only "#!/bin/sh". For some reason this has been ignored by interpreter but as clear error it is better to fix. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

ShriramShastry

Thanks for making the change; they now appear good to me.

singalsu requested review from ShriramShastry, andrula-song and marc-hb May 31, 2023 16:08

marc-hb reviewed May 31, 2023

View reviewed changes

singalsu force-pushed the valgrind_reenable branch from 283934c to db75a33 Compare June 1, 2023 08:04

singalsu marked this pull request as ready for review June 1, 2023 08:04

singalsu requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners June 1, 2023 08:04

singalsu requested review from kv2019i, lgirdwood and marc-hb and removed request for lgirdwood June 1, 2023 08:05

btian1 approved these changes Jun 5, 2023

View reviewed changes

ShriramShastry reviewed Jun 5, 2023

View reviewed changes

kv2019i approved these changes Jun 5, 2023

View reviewed changes

singalsu added 2 commits June 6, 2023 11:23

Tools: Test: Audio: Fix syntax error in test_run.m

127e98f

The fprintf() command prints only "#!/bin/sh". For some reason this has been ignored by interpreter but as clear error it is better to fix. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu force-pushed the valgrind_reenable branch from db75a33 to 127e98f Compare June 6, 2023 08:30

ShriramShastry approved these changes Jun 6, 2023

View reviewed changes

kv2019i merged commit 8696660 into thesofproject:main Jun 6, 2023

	fprintf('Returned status %d\n', ret);
	printf('%s eturned status %d\n', rcmd, ret);

Tools: Test: Audio: Valgrind run related fixes #7719

Tools: Test: Audio: Valgrind run related fixes #7719

Uh oh!

Conversation

singalsu commented May 31, 2023

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShriramShastry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShriramShastry Jun 6, 2023 • edited by marc-hb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShriramShastry left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ShriramShastry Jun 6, 2023 •

edited by marc-hb

Loading