-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove HTTP from supported protocols in Downloader #42
Remove HTTP from supported protocols in Downloader #42
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, wrote just a suggestion
Req. says "download is rejected by SUA and feedback is written to log "
Missing log output.
utest/TestSelfUpdateScenarios.cpp
Outdated
@@ -409,4 +411,21 @@ namespace { | |||
EXPECT_EQ(sentMessages, expectedMessages); | |||
} | |||
|
|||
TEST_F(TestSelfUpdateScenarios, downloadFromHTTPFails_endsInFailedState) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename test like "downloadViaHttpConnectionFails_endsInFailedState"
(in CamelCase names contained acronyms should be written in lowercase to be able to better separate the words while reading)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
utest/TestSelfUpdateScenarios.cpp
Outdated
@@ -409,4 +411,21 @@ namespace { | |||
EXPECT_EQ(sentMessages, expectedMessages); | |||
} | |||
|
|||
TEST_F(TestSelfUpdateScenarios, downloadFromHTTPFails_endsInFailedState) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing TCs for other protocols to be also failing, at least ftp, sftp, smb, smbs, ldap, ldaps
Ok, the task says only "Configure libcurl to not use HTTP protocol", but curl config rejects all protocols now except https. So more protocols should also be tested? (Can't be complete.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added parameterized test case
00055a3
to
a4b0d27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing test case for "http"
src/Download/Downloader.cpp
Outdated
return sua::TechCode::DownloadFailed; | ||
auto e = curl_easy_strerror(res); | ||
sua::Logger::error(e); | ||
return std::make_tuple(sua::TechCode::DownloadFailed, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would add "Errorcode = ...", to get later not an error message with only "Download failed:"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added error to 2nd value in tuple
utest/TestDownloader.cpp
Outdated
TestDownloaderViaUnsupportedProtocols, | ||
TestDownloaderP, | ||
::testing::Values( | ||
"ftp", "sftp", "smb", "smbs", "ldap", "ldaps" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing "http"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 findings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
d933ac6
to
ce8da69
Compare
ce8da69
to
1204cf5
Compare
5f4405a
to
36220fc
Compare
ed5df88
to
384d418
Compare
384d418
to
2804429
Compare
9b69f45
to
1163160
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
No description provided.