Ensure WSDL-generated file parity with webservice.go - #641
Conversation
Signed-off-by: Antonio Gamez Diaz <antonio.gamez@suse.com>
Signed-off-by: Antonio Gamez Diaz <antonio.gamez@suse.com>
| err := s.client.CallContext(ctx, "''", request, &response) | ||
| err := s.client.CallContext(ctx, "''", request, response) |
There was a problem hiding this comment.
Mismatch detected at #630 (comment). I think this is not intended and should be fixed... but perhaps I'm lacking context here.
Is it ok if we change it? @arbulu89
| err := s.client.CallContext(ctx, "''", request, &response) | ||
| err := s.client.CallContext(ctx, "''", request, response) |
There was a problem hiding this comment.
| } | ||
|
|
||
| type HAGetFailoverConfigResponse struct { | ||
| XMLName xml.Name `xml:"urn:SAPControl HAGetFailoverConfigResponse"` |
There was a problem hiding this comment.
In the feature parity test, this is being flagged. I'm lacking context, and I'm not sure if deleting it is OK. See what you think, @arbulu89
Perhaps we could keep it, as it does nothing if the fields does not exist... just in case, but wanted to double-check first.
antgamdia
changed the title
Signed-off-by: Antonio Gamez Diaz <antonio.gamez@suse.com>
|
@arbulu89, the regardless of the tests (we can add them or not, no strong opinions here), I'd like to double-check if the drift is intentional or if it is an actual error: The manually generated file has: err := s.client.CallContext(ctx, "''", request, &response) // note the pointer &but in the autogenerated, the proper signature seems to be: err := s.client.CallContext(ctx, "''", request, response)This is the proposed fix: be5ddcc (I've removed it so that the test actually fails in CI) |
There was a problem hiding this comment.
🟡 Not ready to approve
The implementation still passes &response into CallContext for some operations, which breaks decoding and will cause the newly added parity/unit tests to fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR adds test coverage and drift-detection for the hand-written sapcontrolapi SOAP client to keep it aligned with the WSDL-generated reference, and refreshes contributor guidance for regenerating and validating the WSDL code.
Changes:
- Added a fake-SOAP-server based unit test suite covering the currently supported
WebServiceoperations. - Added an AST-based parity test that compares
webservice.gotypes/method signatures against_generated_wsdl.go. - Updated SAPControl client README with WSDL regeneration and testing instructions; adjusted
HAGetFailoverConfigResponseto match generated XML shape and added a test-friendly constructor.
File summaries
| File | Description |
|---|---|
| internal/core/sapsystem/sapcontrolapi/webservice.go | Removes a drifting XMLName tag from HAGetFailoverConfigResponse and adds NewWebServiceFromClient to allow injecting a SOAP client (used by tests). |
| internal/core/sapsystem/sapcontrolapi/webservice_test.go | Introduces unit tests using httptest to validate request/response decoding for supported operations and basic HTTP error handling. |
| internal/core/sapsystem/sapcontrolapi/parity_test.go | Adds a parity test that parses Go AST to ensure the hand-written subset remains aligned with the generated WSDL reference (types, enums, and response passing to CallContext). |
| internal/core/sapsystem/sapcontrolapi/README.md | Expands contributor documentation for regenerating _generated_wsdl.go, running tests, and adding new operations. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| // NewWebServiceFromClient builds a WebService backed by the given SOAP client. | ||
| // Useful for tests defining their own mocked SOAP client. | ||
| func NewWebServiceFromClient(client *soap.Client) WebService { | ||
| return &webService{client: client} | ||
| } |
| # Generate the code from a running instance (e.g., instance 00) | ||
| ~/go/bin/gowsdl -p sapcontrolapi -o _generated_wsdl.go http://localhost:50013/?wsdl | ||
|
|
||
| # Copy the code from sapcontrol/_generated_wsdl.go to your final destination |
| ## References | ||
|
|
||
| - [How to user the SAPControl Web Service Interface](https://www.sap.com/documents/2016/09/0a40e60d-8b7c-0010-82c7-eda71af511fa.html) No newline at end of file | ||
| * [How to user the SAPControl Web Service Interface](https://www.sap.com/documents/2016/09/0a40e60d-8b7c-0010-82c7-eda71af511fa.html) |
| ## Testing | ||
|
|
||
| * `webservice_test.go` runs each hand-written operation against a fake SOAP server and checks the response decodes correctly. | ||
| * `parity_test.go`'s ensures `webservice.go`'s hand-written subset is in sync with the autogenerated `_generated_wsdl.go`. If new operations are added to the WSDL, this test will not fail, but it will log the new operations that are not yet wrapped in `webservice.go`. |
There was a problem hiding this comment.
Hey @antgamdia ,
Do we need to touch these thing?
I personally would vote to keep as it is. I find really little value, just add tests to "cover" code. I don't think we need to test auto-generated code.
arbulu89
left a comment
arbulu89
left a comment
There was a problem hiding this comment.
Green light!
I guess you will change the %response to response in the pointers that are wrong before mergint
|
As shared offline, it turns out the double-pointer issue is, thankfully, harmless IRL due to a guard in the XML decoder: func (d *Decoder) DecodeElement(v any, start *StartElement) error {
val := reflect.ValueOf(v)
if val.Kind() != reflect.Pointer { return errors.New("non-pointer passed to Unmarshal") }
if val.IsNil() { return errors.New("nil pointer passed to Unmarshal") }
return d.unmarshal(val.Elem(), start, 0) // <-- dereferences ONE level, unconditionally
}
func (d *Decoder) unmarshal(val reflect.Value, start *StartElement, depth int) error {
...
if val.Kind() == reflect.Pointer {
if val.IsNil() { val.Set(reflect.New(val.Type().Elem())) }
val = val.Elem() // <-- dereferences again, only if still a pointer
}
// from here val is finally the Struct, whichever path got here |
Description
This pull request introduces comprehensive unit tests for the hand-written Go client for the SAPControl web service, and significantly improves the documentation for contributing to and testing the package. The new tests use a fake SOAP server to verify correct request/response handling for each supported operation and ensure the client’s implementation remains in sync with the auto-generated WSDL code. The updated
README.mdnow provides detailed instructions on regenerating the WSDL code, running tests, and adding new operations.webservice_test.gowith unit tests for all supported SAPControl operations, using a fake SOAP server to validate request and response handling. This ensures that the client correctly decodes SOAP responses and handles errors as expected.How was this tested?
UT
Documentation changes
No
Additional information
See example failure (before the fix):
Issues in the function calls:
Operations not in webservice.go:
=== NAME TestFeatureParityWithGeneratedWSDL Error: parity_test.go:79: 109/119 gowsdl operations are not wrapped by webservice.go: ABAPAcknowledgeAlertsContext, ABAPCheckRFCDestinationsContext, ABAPGetComponentListContext, ABAPGetSystemWPTableContext, ABAPGetWPTableContext, ABAPReadRawSyslogContext, ABAPReadSyslogContext, ABAPSetServerInactiveContext, AccessCheckContext, AnalyseLogFilesContext, BootstrapContext, CheckPSEContext, CheckParameterContext, CheckUpdateSystemContext, ConfigureLogFileListContext, CreatePSECredentialContext, CreateSnapshotContext, DeletePSEContext, DeleteSnapshotsContext, EnqGetLockTableContext, EnqGetStatisticContext, EnqRemoveLocksContext, EnqRemoveUserLocksContext, GWCancelConnectionsContext, GWDeleteClientsContext, GWDeleteConnectionsContext, GWGetClientListContext, GWGetConnectionListContext, GetAccessPointListContext, GetAlertTreeContext, GetAlertsContext, GetCallstackContext, GetEnvironmentContext, GetLogFileListContext, GetNetworkIdContext, GetProcessParameterContext, GetQueueStatisticContext, GetSecNetworkIdContext, GetStartProfileContext, GetSystemUpdateListContext, GetTraceFileContext, HACheckFailoverConfigContext, HACheckMaintenanceModeContext, HAFailoverToNodeContext, HASetMaintenanceModeContext, ICMGetCacheEntriesContext, ICMGetConnectionListContext, ICMGetProxyConnectionListContext, ICMGetThreadListContext, InstanceStartContext, InstanceStopContext, J2EEControlClusterContext, J2EEControlComponentsContext, J2EEControlProcessContext, J2EEDisableDbgSessionContext, J2EEEnableDbgSessionContext, J2EEGetApplicationAliasListContext, J2EEGetCacheStatistic2Context, J2EEGetCacheStatisticContext, J2EEGetClusterMsgListContext, J2EEGetComponentListContext, J2EEGetEJBSessionListContext, J2EEGetProcessList2Context, J2EEGetProcessListContext, J2EEGetRemoteObjectListContext, J2EEGetSessionListContext, J2EEGetSharedTableInfoContext, J2EEGetThreadCallStackContext, J2EEGetThreadList2Context, J2EEGetThreadListContext, J2EEGetThreadTaskStackContext, J2EEGetVMGCHistory2Context, J2EEGetVMGCHistoryContext, J2EEGetVMHeapInfoContext, J2EEGetWebSessionList2Context, J2EEGetWebSessionListContext, ListConfigFilesContext, ListDeveloperTracesContext, ListLogFilesContext, ListSnapshotsContext, OSExecuteContext, ParameterValueContext, ReadConfigFileContext, ReadDeveloperTraceContext, ReadLogFileContext, ReadSnapshotContext, RequestLogonFileContext, RequestTicketContext, RestartInstanceContext, RestartServiceContext, RestartSystemContext, SendSignalContext, SendTicketContext, SetProcessParameter2Context, SetProcessParameterContext, ShmDetachContext, ShutdownContext, StartBypassHAContext, StopBypassHAContext, StopServiceContext, StorePSEContext, UpdateInstancePSEContext, UpdateSCSInstanceContext, UpdateSystemContext, UpdateSystemPKIContext, WebDispGetGroupListContext, WebDispGetServerListContext, WebDispGetUrlPrefixListContext, WebDispGetVirtHostListContext