| Message ID | cover.1730207790.git.echaudro@redhat.com |
|---|---|
| Headers | show |
| Series | tests: Replace wget with curl. | expand |
On Tue, Oct 29, 2024 at 2:33 PM Eelco Chaudron <echaudro@redhat.com> wrote: > > Eelco Chaudron (2): > system-traffic: Replace wget with curl for negative and ftp tests. > system-traffic: Standardize by replacing all wget instances with curl. > > tests/system-tap.at | 3 +- > tests/system-traffic.at | 249 +++++++++++++++++++++++++++------------- > 2 files changed, 172 insertions(+), 80 deletions(-) > Overall, it lgtm (just a nit in the second patch).
On 30 Oct 2024, at 15:34, David Marchand wrote: > On Tue, Oct 29, 2024 at 2:33 PM Eelco Chaudron <echaudro@redhat.com> wrote: >> >> Eelco Chaudron (2): >> system-traffic: Replace wget with curl for negative and ftp tests. >> system-traffic: Standardize by replacing all wget instances with curl. >> >> tests/system-tap.at | 3 +- >> tests/system-traffic.at | 249 +++++++++++++++++++++++++++------------- >> 2 files changed, 172 insertions(+), 80 deletions(-) >> > > Overall, it lgtm (just a nit in the second patch). Thanks for the review. I can fix the nit on commit. Let me know if you want to add your ack/review-by. //Eelco
On Wed, Oct 30, 2024 at 3:45 PM Eelco Chaudron <echaudro@redhat.com> wrote: > On 30 Oct 2024, at 15:34, David Marchand wrote: > > > On Tue, Oct 29, 2024 at 2:33 PM Eelco Chaudron <echaudro@redhat.com> wrote: > >> > >> Eelco Chaudron (2): > >> system-traffic: Replace wget with curl for negative and ftp tests. > >> system-traffic: Standardize by replacing all wget instances with curl. > >> > >> tests/system-tap.at | 3 +- > >> tests/system-traffic.at | 249 +++++++++++++++++++++++++++------------- > >> 2 files changed, 172 insertions(+), 80 deletions(-) > >> > > > > Overall, it lgtm (just a nit in the second patch). > > Thanks for the review. I can fix the nit on commit. Let me know if you want to add your ack/review-by. Ah yes, you can add on both patches. Acked-by: David Marchand <david.marchand@redhat.com>
Eelco Chaudron <echaudro@redhat.com> writes: > Eelco Chaudron (2): > system-traffic: Replace wget with curl for negative and ftp tests. > system-traffic: Standardize by replacing all wget instances with curl. > > tests/system-tap.at | 3 +- > tests/system-traffic.at | 249 +++++++++++++++++++++++++++------------- > 2 files changed, 172 insertions(+), 80 deletions(-) > > -- Acked-by: Paolo Valerio <pvalerio@redhat.com>
On 10/29/24 14:32, Eelco Chaudron wrote: > Eelco Chaudron (2): > system-traffic: Replace wget with curl for negative and ftp tests. > system-traffic: Standardize by replacing all wget instances with curl. > > tests/system-tap.at | 3 +- > tests/system-traffic.at | 249 +++++++++++++++++++++++++++------------- > 2 files changed, 172 insertions(+), 80 deletions(-) > Hi, Eelco. Thanks for the patches! I didn't run the tests, but I have a couple of high level comments: 1. It might be better if we introduce a macro for calling curl. Since most tests are using the same list of arguments, it's better if we manage them in a single place, so in case of a needed change, we'll just change in the macro instead of touching all the tests again. Maybe something like: OVS_GET_HTTP([URL]) OVS_GET_FTP([URL]) May also be a good time to unify timouts and retries in all the calls. 2. Instead of >curl0.log 2>&1, it might be better to use [0], [ignore], [ignore]. We don't seem to check the log anyway most of the time, and with [ignore] it will show up directly in the testsuite log. Can use [stdout], [stderr] in case the output needs checking in the test. (Not sure what will be printed when we download binary files though, didn't check.) 3. We install wget in CI and also Documentation/intro/install/general.rst lists it as a testing dependency. Should be cleaned up, I think. WDYT? Best rgeards, Ilya Maximets.