expanded discovery calls #350
Conversation
| nock.disableNetConnect(); | ||
| nock(service.url) | ||
| .persist() | ||
| .post(paths.environments + '?version=' + service.version_date) |
There was a problem hiding this comment.
I prefer individual nock()'s per test but not a blocker. (And I know a ton of our existing tests use this pattern...)
There was a problem hiding this comment.
Yeah, I copied this from the v1-experimental tests.
There was a problem hiding this comment.
Yea.. after I looked more thoroughly, these aren't even being used because the tests are all sync - see my later comments
| * Initial release | ||
| * @type {string} | ||
| */ | ||
| DiscoveryV1.VERSION_DATE_2016_07_11 = '2016-11-07'; |
There was a problem hiding this comment.
Is this the correct version_date for the GA release?
| * | ||
| * @param {Object} params | ||
| * @param {String} params.environment_id | ||
| * @param {String} [params.name] Find collections with the given name. |
There was a problem hiding this comment.
I don't see params.name used anywhere in this method.
(It looks like this is also wrong in https://github.com/watson-developer-cloud/node-sdk/blob/master/discovery/v1-experimental.js#L103 )
There was a problem hiding this comment.
fixed. and it's right in experimental.
| * @param params | ||
| * @param {String} params.environment_id environment guid for the collection | ||
| * @param {string} params.collection_id the guid of the collection to delete | ||
| * @param {file} params.file a file to post (smaller than 50mb) |
There was a problem hiding this comment.
I think this should be {Buffer|ReadableStream|Object} rather than {file} (?)
(Object allows you to specify filename and/or content type, e.g. {value: Buffer, options: contentType: 'text/foo'}.. or something like that.)
| * @param {string} params.collection_id the guid of the collection to delete | ||
| * @param {file} params.file a file to post (smaller than 50mb) | ||
| * @param {string} [params.configuration_id] config guid | ||
| * @param {string} [params.metadata] file metadata, including content-type (will infer if missing) |
There was a problem hiding this comment.
What should params.metada look like?
There was a problem hiding this comment.
they should look at the docs. at least isn't that what the other discussion concluded?
|
|
||
| util.inherits(DiscoveryV1, BaseService); | ||
| DiscoveryV1.prototype.name = 'discovery'; | ||
| DiscoveryV1.prototype.version = 'v1-experimental'; |
There was a problem hiding this comment.
BTW, I'm not seeing the change in the PR - perhaps you put it in a different branch or forgot to push it to github?
| util.inherits(DiscoveryV1, BaseService); | ||
| DiscoveryV1.prototype.name = 'discovery'; | ||
| DiscoveryV1.prototype.version = 'v1-experimental'; | ||
| DiscoveryV1.URL = 'https://gateway.watsonplatform.net/discovery-experimental/api'; |
There was a problem hiding this comment.
Presumably this shouldn't have -experimental in the URL either (?)
| }); | ||
|
|
||
| it('should check no parameters provided (negative test)', function() { | ||
| discovery.getEnvironments({}, missingParameter); |
There was a problem hiding this comment.
This test doesn't make any sense. It passes regardless of weather or not getEnvironments() throws a missingParameter error. (Because the test is sync, but if getEnvironments doesn't throw an error, then fires the callback async - so the test has already passed by the time missingParameter throws). I think we should probably make the callback always async, and make missingParameter accept a callback.
There was a problem hiding this comment.
I've no idea what that was doing there. removed.
| threw = true; | ||
| assert.equal(err.message, 'Argument error: version_date was not specified, use DiscoveryV1.VERSION_DATE_2016_07_11'); | ||
| } | ||
| assert(threw, 'should throw an error') |
There was a problem hiding this comment.
BTW, there is an assert.throws method ;)
https://nodejs.org/api/assert.html#assert_assert_throws_block_error_message
There was a problem hiding this comment.
good to know. I copied that pattern from another test. fixed.
| .delete(paths.delete_document + '?version=' + service.version_date) | ||
| .reply(200, {'delete_doc': 'yes'}) | ||
| .get(paths.configurations + '?version=' + service.version_date) | ||
| .reply(200, {'configs': 'yes'}); |
There was a problem hiding this comment.
FYI: these don't hurt, but nock can't check things in time for for sync tests because the test passes before nock gets called, and any error that nock might throw gets swallowed by mocha because it occurs outside of a test :/
|
|
||
| util.inherits(DiscoveryV1, BaseService); | ||
| DiscoveryV1.prototype.name = 'discovery'; | ||
| DiscoveryV1.prototype.version = 'v1-experimental'; |
There was a problem hiding this comment.
BTW, I'm not seeing the change in the PR - perhaps you put it in a different branch or forgot to push it to github?
|
@nfriedly I did miss a file. it's in now. |
|
Should be released in |
Adds ability to create collections and add documents