Initial source code #1
Conversation
tests/server_test.php
Outdated
| PARAM_BOOL, '', VALUE_REQUIRED); | ||
| } | ||
|
|
||
| public static function function($params) {} |
There was a problem hiding this comment.
This is meant to mirror the upsert() method in auth/avadosalesforce/classes/external/upsert_v1.php, right? But do we need to define it here inside this class? And if we do, can it use a more telling name, as 'function' is pretty generic?
There was a problem hiding this comment.
Yes in the sense that it's the implementation of a WS function, but no as they're separate plugins -- this is just a mock. If you have a better name I'll take it, but I'm keen to follow the naming convention in the external functions API.
There was a problem hiding this comment.
I'd go with mock_upsert_parameters, mock_upsert_returns and mock_upsert, but it's not a life-and-death issue.
There was a problem hiding this comment.
I'll meet you halfway with mock_external_function, since this is not directly related to auth_avadosalesforce.
| $server = new webservice_loadedrest_server_mock($mockformat, $inputfile->url()); | ||
| $server->run(); | ||
| } | ||
| } |
There was a problem hiding this comment.
And we need to add some assertions to this test, as there currently are none.
There was a problem hiding this comment.
The test is intended to cover server's interactions with format, which is achieved via the PHPUnit InvokedAtIndex matcher. I can't see any value in adding additional assertions?
There was a problem hiding this comment.
OK, got it. It's just that when run, PHPUnit doesn't output any sort of status for those - it is like you had no tests whatsoever (no dots, no other notification etc.) But it does not fail.
| public function data_send_response() { | ||
| return [ | ||
| [ | ||
| 'result' => true, |
There was a problem hiding this comment.
Should we provide a key-value array as a value for 'result' here, like it happens below? As in, 'result' => ['key' => true].
There was a problem hiding this comment.
This is tested in the data block immediately below -- this one's to test external_values in the root of the request.
| /** | ||
| * @dataProvider data_send_response | ||
| */ | ||
| public function test_send_response($result, external_description $description, $expect) { |
There was a problem hiding this comment.
When running this test, I get this warning:
1) Warning The data provider specified for webservice_loadedrest_xml_format_testcase::test_send_response is invalid. Class 'external_value' not found
Doesn't quite make sense, as new external_value() statements inside data_send_response() easily lead to /lib/externallib.php from inside PhpStorm.
There was a problem hiding this comment.
Can you paste the output of $ vendor/bin/phpunit --group webservice_loadedrest here? I'm interested in seeing the version information.
There was a problem hiding this comment.
Here it is:
Moodle 3.4.2+ (Build: 20180405)
Php: 7.0.30.0.0.16.04.1, sqlsrv: 14.00.1000, OS: Linux 4.4.0-127-generic x86_64
PHPUnit 6.5.7 by Sebastian Bergmann and contributors.
......
There was a problem hiding this comment.
Fixed via include of externallib -- it seems data providers execute prior to the PHPUnit bootstrap, which means the classloader isn't around to load the missing external_* classes.
|
Fixed the above and added a |
Tired (will open issues after merging):
serialise()implementation for it. Given that the client is currently unused, I suggest we avoid this for now. (XML support for the web service client #2)external_multiple_structurevalues still persists. The current mitigation is to just add a unique identifier to the end of the tag, e.g. an int we just increment each time. This is left to the developer. (XML request payloads can't contain tags of the same name #3)Wired:
PUT!).Inspired: cleaned up Git history.