Re: [ndctl PATCH v3] test/monitor.sh: replace sleep with event driven wait

[Alison Schofield <alison.schofield@intel.com>]

On Mon, May 19, 2025 at 03:42:30PM -0700, Marc Herbert wrote:
> On 2025-05-19 12:28, alison.schofield@intel.com wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > 
> > Again, looking for Tested-by Tags. Thanks!
> 
> Wow, it went so fast I genuinely thought something went terribly wrong
> and it did not test anything anymore. While reporting "Success!" - that
> has happened before.
> 
> But I check the test logs and it does a lot of stuff - in less 2
> seconds! What a difference.
> 
> Tested-by: Marc Herbert <marc.herbert@linux.intel.com>
> 
> I have only concern holding back my Reviewed-by + some minor nits after
> the code. I think the error message for the timeout pipeline is too
> limited, terse and generic; does not say anything about what happened,
> does not make the difference between a timeout and some other, unexpected
> failure, how many occurences were found etc. I think the error clause
> should do at least two things:
> 
> - print the timeout $? exit code.

OK. I'll combine that request with what Vishal suggested
	set -o pipefail
	do my 'do or die' cmd
		check the rc for timeout, grep no match, or other.

> - grep the log file again, either with -c if it's too long, or not.
> - Any other useful information that could be up for grabs at that point.
> 

I can add dump the logfile to the test log before removing it,
everytime. Each test case only puts in 1-4 entries. 

> 
> > +wait_for_logfile_update()
> > +{
> > +	local expect_string="$1"
> > +	local expect_count="$2"
> > +
> > +	# Wait up to 3s for $expect_count occurrences of $expect_string
> > +	# tail -n +1 -F: starts watching the logfile from the first line
> > +
> > +	if ! timeout 3s tail -n +1 -F "$logfile" | grep -m "$expect_count" -q "$expect_string"; then
> > +		echo "logfile not updated in 3 secs"
> > +		err "$LINENO"
> > +	fi
> > +}
> 
> tail -F really does solve the problem with very little code, well
> spotted!
> Nit 1: I did not know that -F option: can you try to sneak the word
> "retry" somewhere in the comments?

will do

> 
> Codestyle Nit 2: to avoid negations I generally prefer:
> 
>   timeout 3s tail -n +1 -F "$logfile" |
>      grep -q -m "$expect_count" -q "$expect_string" || {
>       error ...
>     }
> 
> This also translates to plain English nicely as "wait or fail", "do or
> die", etc.
> 

You prefer do or die style. I can do that.


> Super minor nit 3: "$expect_string" looks like an argument of "-q", can
> you move -q first?

will do.