diff mbox

[FFmpeg-devel] tests: Fix bash errors in lavf_container tests.

Message ID 20190828153237.30155-1-andrey.semashev@gmail.com
State Accepted
Commit 6d9d053edbedbb0d9fc0a97141b7524d8658be58
Headers show

Commit Message

Andrey Semashev Aug. 28, 2019, 3:32 p.m. UTC
Because lavf_container is sometimes called with only 2 arguments,
fate tests produce bash errors like this:

  tests/fate-run.sh: 299: test: =: unexpected operator

This commit fixes this.
---
 tests/fate-run.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrey Semashev Sept. 2, 2019, 10:54 a.m. UTC | #1
Ping?

On 2019-08-28 18:32, Andrey Semashev wrote:
> Because lavf_container is sometimes called with only 2 arguments,
> fate tests produce bash errors like this:
>
>    tests/fate-run.sh: 299: test: =: unexpected operator
>
> This commit fixes this.
> ---
>   tests/fate-run.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/fate-run.sh b/tests/fate-run.sh
> index 2f1991da52..aec12c16a3 100755
> --- a/tests/fate-run.sh
> +++ b/tests/fate-run.sh
> @@ -296,7 +296,7 @@ lavf_container(){
>       outdir="tests/data/lavf"
>       file=${outdir}/lavf.$t
>       do_avconv $file $DEC_OPTS -f image2 -c:v pgmyuv -i $raw_src $DEC_OPTS -ar 44100 -f s16le $1 -i $pcm_src "$ENC_OPTS -metadata title=lavftest" -b:a 64k -t 1 -qscale:v 10 $2
> -    test $3 = "disable_crc" ||
> +    test "$3" = "disable_crc" ||
>           do_avconv_crc $file $DEC_OPTS -i $target_path/$file $3
>   }
>
Andrey Semashev Sept. 6, 2019, 7:26 p.m. UTC | #2
Could someone take a look at this small fix, please? I'd like it to make 
it into 4.2.1.

On 2019-09-02 13:54, Andrey Semashev wrote:
> Ping?
> 
> On 2019-08-28 18:32, Andrey Semashev wrote:
>> Because lavf_container is sometimes called with only 2 arguments,
>> fate tests produce bash errors like this:
>>
>>    tests/fate-run.sh: 299: test: =: unexpected operator
>>
>> This commit fixes this.
>> ---
>>   tests/fate-run.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/fate-run.sh b/tests/fate-run.sh
>> index 2f1991da52..aec12c16a3 100755
>> --- a/tests/fate-run.sh
>> +++ b/tests/fate-run.sh
>> @@ -296,7 +296,7 @@ lavf_container(){
>>       outdir="tests/data/lavf"
>>       file=${outdir}/lavf.$t
>>       do_avconv $file $DEC_OPTS -f image2 -c:v pgmyuv -i $raw_src 
>> $DEC_OPTS -ar 44100 -f s16le $1 -i $pcm_src "$ENC_OPTS -metadata 
>> title=lavftest" -b:a 64k -t 1 -qscale:v 10 $2
>> -    test $3 = "disable_crc" ||
>> +    test "$3" = "disable_crc" ||
>>           do_avconv_crc $file $DEC_OPTS -i $target_path/$file $3
>>   }
> 
>
Michael Niedermayer Sept. 7, 2019, 3:19 p.m. UTC | #3
On Wed, Aug 28, 2019 at 06:32:37PM +0300, Andrey Semashev wrote:
> Because lavf_container is sometimes called with only 2 arguments,
> fate tests produce bash errors like this:
> 
>   tests/fate-run.sh: 299: test: =: unexpected operator
> 
> This commit fixes this.
> ---
>  tests/fate-run.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I think this change is correct but shell is not my area ...

[...]
Limin Wang Sept. 7, 2019, 3:32 p.m. UTC | #4
On Sat, Sep 07, 2019 at 05:19:55PM +0200, Michael Niedermayer wrote:
> On Wed, Aug 28, 2019 at 06:32:37PM +0300, Andrey Semashev wrote:
> > Because lavf_container is sometimes called with only 2 arguments,
> > fate tests produce bash errors like this:
> > 
> >   tests/fate-run.sh: 299: test: =: unexpected operator
> > 
> > This commit fixes this.
> > ---
> >  tests/fate-run.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I think this change is correct but shell is not my area ...
If the $2 is two arguments between with space, the patch is needed.
Why I haven't got such errors when I'm running fate?

> 
> [...]
> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Its not that you shouldnt use gotos but rather that you should write
> readable code and code with gotos often but not always is less readable



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andrey Semashev Sept. 7, 2019, 9:46 p.m. UTC | #5
On 2019-09-07 18:32, Limin Wang wrote:
> On Sat, Sep 07, 2019 at 05:19:55PM +0200, Michael Niedermayer wrote:
>> On Wed, Aug 28, 2019 at 06:32:37PM +0300, Andrey Semashev wrote:
>>> Because lavf_container is sometimes called with only 2 arguments,
>>> fate tests produce bash errors like this:
>>>
>>>    tests/fate-run.sh: 299: test: =: unexpected operator
>>>
>>> This commit fixes this.
>>> ---
>>>   tests/fate-run.sh | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> I think this change is correct but shell is not my area ...
> If the $2 is two arguments between with space, the patch is needed.

$2 is a single argument that may contain spaces. It is interpreted as a 
single argument as long as it is enclosed in quotes in lavf_container 
invokation.

The problem is caused not by $2 but by the fact there is no $3. See 
lavf_container_attach, lavf_container_timecode_nodrop, 
lavf_container_timecode_drop and lavf_container_timecode functions, as 
well as a few tests in tests/fate/lavf-container.mak.

> Why I haven't got such errors when I'm running fate?

I noticed the errors when the tests failed (due to my local changes not 
relevant to this patch). I don't know the details about how tests are 
run, but maybe the output is suppressed when the tests pass?
Limin Wang Sept. 8, 2019, 10:45 a.m. UTC | #6
On Sun, Sep 08, 2019 at 12:46:25AM +0300, Andrey Semashev wrote:
> On 2019-09-07 18:32, Limin Wang wrote:
> >On Sat, Sep 07, 2019 at 05:19:55PM +0200, Michael Niedermayer wrote:
> >>On Wed, Aug 28, 2019 at 06:32:37PM +0300, Andrey Semashev wrote:
> >>>Because lavf_container is sometimes called with only 2 arguments,
> >>>fate tests produce bash errors like this:
> >>>
> >>>   tests/fate-run.sh: 299: test: =: unexpected operator
> >>>
> >>>This commit fixes this.
> >>>---
> >>>  tests/fate-run.sh | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>I think this change is correct but shell is not my area ...
> >If the $2 is two arguments between with space, the patch is needed.
> 
> $2 is a single argument that may contain spaces. It is interpreted
> as a single argument as long as it is enclosed in quotes in
> lavf_container invokation.
> 
> The problem is caused not by $2 but by the fact there is no $3. See
> lavf_container_attach, lavf_container_timecode_nodrop,
> lavf_container_timecode_drop and lavf_container_timecode functions,
> as well as a few tests in tests/fate/lavf-container.mak.
> 
> >Why I haven't got such errors when I'm running fate?
> 
> I noticed the errors when the tests failed (due to my local changes
> not relevant to this patch). I don't know the details about how
> tests are run, but maybe the output is suppressed when the tests
> pass?
The patch looks good to me, but I don't have push access. Please ask for
other developer to push it.


> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Sept. 9, 2019, 4:16 p.m. UTC | #7
On Sun, Sep 08, 2019 at 06:45:30PM +0800, Limin Wang wrote:
> On Sun, Sep 08, 2019 at 12:46:25AM +0300, Andrey Semashev wrote:
> > On 2019-09-07 18:32, Limin Wang wrote:
> > >On Sat, Sep 07, 2019 at 05:19:55PM +0200, Michael Niedermayer wrote:
> > >>On Wed, Aug 28, 2019 at 06:32:37PM +0300, Andrey Semashev wrote:
> > >>>Because lavf_container is sometimes called with only 2 arguments,
> > >>>fate tests produce bash errors like this:
> > >>>
> > >>>   tests/fate-run.sh: 299: test: =: unexpected operator
> > >>>
> > >>>This commit fixes this.
> > >>>---
> > >>>  tests/fate-run.sh | 2 +-
> > >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >>I think this change is correct but shell is not my area ...
> > >If the $2 is two arguments between with space, the patch is needed.
> > 
> > $2 is a single argument that may contain spaces. It is interpreted
> > as a single argument as long as it is enclosed in quotes in
> > lavf_container invokation.
> > 
> > The problem is caused not by $2 but by the fact there is no $3. See
> > lavf_container_attach, lavf_container_timecode_nodrop,
> > lavf_container_timecode_drop and lavf_container_timecode functions,
> > as well as a few tests in tests/fate/lavf-container.mak.
> > 
> > >Why I haven't got such errors when I'm running fate?
> > 
> > I noticed the errors when the tests failed (due to my local changes
> > not relevant to this patch). I don't know the details about how
> > tests are run, but maybe the output is suppressed when the tests
> > pass?
> The patch looks good to me, but I don't have push access. Please ask for
> other developer to push it.

will apply

thx


[...]
diff mbox

Patch

diff --git a/tests/fate-run.sh b/tests/fate-run.sh
index 2f1991da52..aec12c16a3 100755
--- a/tests/fate-run.sh
+++ b/tests/fate-run.sh
@@ -296,7 +296,7 @@  lavf_container(){
     outdir="tests/data/lavf"
     file=${outdir}/lavf.$t
     do_avconv $file $DEC_OPTS -f image2 -c:v pgmyuv -i $raw_src $DEC_OPTS -ar 44100 -f s16le $1 -i $pcm_src "$ENC_OPTS -metadata title=lavftest" -b:a 64k -t 1 -qscale:v 10 $2
-    test $3 = "disable_crc" ||
+    test "$3" = "disable_crc" ||
         do_avconv_crc $file $DEC_OPTS -i $target_path/$file $3
 }