-
Notifications
You must be signed in to change notification settings - Fork 14
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
created a test class header for globus api #1049
base: 1044-bug-fix
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThe changes introduce a new test class Class diagram for GlobusAPI and TestGlobusAPIclassDiagram
class GlobusAPI {
-void init()
protected long get(CURL *a_curl, const std::string &a_base_url, const std::string &a_url_path, const std::string &a_token, const std::vector<std::pair<std::string, std::string>> &a_params, std::string &a_result)
protected long post(CURL *a_curl, const std::string &a_base_url, const std::string &a_url_path, const std::string &a_token, const std::vector<std::pair<std::string, std::string>> &a_params, const libjson::Value *a_body, std::string &a_result)
private std::string getSubmissionID(const std::string &a_acc_token)
private bool eventsHaveErrors(const std::vector<std::string> &a_events, XfrStatus &status, std::string &a_err_msg)
}
class TestGlobusAPI {
+TestGlobusAPI()
+~TestGlobusAPI()
+long get(CURL *a_curl, const std::string &a_base_url, const std::string &a_url_path, const std::string &a_token, const std::vector<std::pair<std::string, std::string>> &a_params, std::string &a_result)
+long post(CURL *a_curl, const std::string &a_base_url, const std::string &a_url_path, const std::string &a_token, const std::vector<std::pair<std::string, std::string>> &a_params, const libjson::Value *a_body, std::string &a_result)
}
GlobusAPI <|-- TestGlobusAPI
note for GlobusAPI "The init method access specifier changed from private to protected."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @nedvedba - I've reviewed your changes - here's some feedback:
Overall Comments:
- The constructor and destructor should be declared in the public section of the class
- Missing public inheritance specifier - should be
class TestGlobusAPI : public GlobusAPI
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
core/server/TestGlobusAPI.hpp
Outdated
namespace SDMS { | ||
namespace Core { | ||
|
||
class TestGlobusAPI: GlobusAPI { |
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.
issue: Missing public inheritance specifier and constructor/destructor should be public
The class should use 'public' inheritance (': public GlobusAPI'), and the constructor/destructor should be moved under a public access specifier to allow instantiation of the test class.
d135af7
to
9acb26c
Compare
Just fix unit test issue, and run par herems formatter. |
@sourcery-ai review |
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.
Hey @nedvedba - I've reviewed your changes - here's some feedback:
Overall Comments:
- There's a syntax error in TestGlobusAPI constructor - missing semicolon after GlobusAPI::init()
- TestGlobusAPI api() declares a function instead of creating an object - remove the parentheses to fix the most vexing parse
- Consider making init() protected instead of private since derived test class needs to call it
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
class TestGlobusAPI: public GlobusAPI { | ||
public: | ||
TestGlobusAPI() { | ||
GlobusAPI::init() | ||
} | ||
|
||
~TestGlobusAPI() {} | ||
|
||
long get(CURL *a_curl, const std::string &a_base_url, | ||
const std::string &a_url_path, const std::string &a_token, |
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.
suggestion (testing): Test class could benefit from mock methods for better testing isolation
Consider adding mock methods to override the actual HTTP calls in the test class. This would allow testing the API without making real network requests and enable testing of error scenarios. You could use dependency injection for the CURL handle to facilitate this.
class TestGlobusAPI: public GlobusAPI {
public:
TestGlobusAPI(): mock_response_code_(200) {}
~TestGlobusAPI() = default;
void set_mock_response(long code, const std::string& response) {
mock_response_code_ = code;
mock_response_ = response;
}
long get(CURL*, const std::string&, const std::string&, const std::string&,
const std::vector<std::pair<std::string, std::string>>&, std::string& result) override {
result = mock_response_;
return mock_response_code_;
}
long post(CURL*, const std::string&, const std::string&, const std::string&,
const std::vector<std::pair<std::string, std::string>>&,
const libjson::Value*, std::string& result) override {
result = mock_response_;
return mock_response_code_;
}
private:
long mock_response_code_;
std::string mock_response_;
};
TestGlobusAPI() { | ||
GlobusAPI::init() |
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.
issue (testing): Missing semicolon after init() call and no error handling in constructor
The init() call is missing a semicolon. Additionally, consider adding error handling and a test case that verifies proper initialization and handling of initialization failures.
Summary by Sourcery
Add a test class
TestGlobusAPI
to enable testing of theGlobusAPI
class by exposing itsget
andpost
methods. Modify theinit
method's access level inGlobusAPI
to protected to support subclassing.Enhancements:
init
method inGlobusAPI
from private to protected to allow subclassing.Tests:
TestGlobusAPI
to facilitate testing of theGlobusAPI
class by providing public access to itsget
andpost
methods.