On Mon, May 19, 2025 at 03:42:30PM -0700, Marc Herbert wrote:
> On 2025-05-19 12:28, alison.schofi...@intel.com wrote:
> > From: Alison Schofield <alison.schofi...@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.herb...@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.
