TritonDataCenter / sdc-cloudapi

Triton Data Center Public HTTP API
Mozilla Public License 2.0
10 stars 24 forks source link

Various fixes for cloudapi test suite, and fix role-related 500 caused by double-callback in mahi #88

Closed marsell closed 3 years ago

marsell commented 3 years ago

Some notes:

package.json uses tape 5.0.0 and above, but tape switched deep-equal library versions between 4.x and 5.x, and the new deep-equal has some strange behaviour: deepEqual() failing with objects that are semantically identical, but have a different ordering of attributes. For example:

    operator: deepEqual
    expected: |-
      { jse_info: {}, jse_shortmsg: '', message: 'Role(s) asdasdasdasd not found', statusCode: 409, restCode: 'InvalidArgument', name: 'InvalidArgumentError', body: { code: 'InvalidArgument', message: 'Role(s) asdasdasdasd not found' } }
    actual: |-
      { jse_shortmsg: '', jse_info: {}, message: 'Role(s) asdasdasdasd not found', statusCode: 409, body: { code: 'InvalidArgument', message: 'Role(s) asdasdasdasd not found' }, restCode: 'InvalidArgument', name: 'InvalidArgumentError' }
    at: <anonymous> (/opt/smartdc/cloudapi/test/auth.test.js:467:11)
    stack: |-
      InvalidArgumentError: Role(s) asdasdasdasd not found
          at parseResponse (/opt/smartdc/cloudapi/node_modules/restify/lib/clients/json_client.js:71:23)
          at IncomingMessage.done (/opt/smartdc/cloudapi/node_modules/restify/lib/clients/string_client.js:167:13)
          at IncomingMessage.g (events.js:292:16)
          at emitNone (events.js:91:20)
          at IncomingMessage.emit (events.js:185:7)
          at endReadableNT (_stream_readable.js:978:12)
          at _combinedTickCallback (internal/process/next_tick.js:80:11)
          at process._tickCallback (internal/process/next_tick.js:104:9)

I could fix the tests themselves, but since this new deepEqual() behaviour is nonsense, switch to using the older tape 4.13.2 instead.

Separately, there is a nics.test.js test failing due to a double-callback:

Uncaught AssertionError: idx should be equal to ndone

FROM
fail (assert.js:84:3)
Function.equal (assert.js:104:27)
next (/opt/smartdc/cloudapi/node_modules/vasync/lib/vasync.js:805:14)
/opt/smartdc/cloudapi/test/nics.test.js:617:21
/opt/smartdc/cloudapi/test/common.js:1113:13
/opt/smartdc/cloudapi/node_modules/sdc-clientsok 11 deleteNicTag sdccloudapitest_nics_nictag1
/lib/restifyclient.js:121:20
parseResponse (/opt/smartdc/cloudapi/node_modules/sdc-clients/node_modules/restify-clients/lib/JsonClient.js:105:9)
IncomingMessage.done (/opt/smartdc/cloudapi/node_modules/sdc-clients/node_modules/restify-clients/lib/StringClient.js:237:13)
IncomingMessage.g (events.js:292:16)
emitNone (events.js:91:20)
IncomingMessage.emit (events.js:185:7)
endReadableNT (_stream_readable.js:978:12)
_combinedTickCallback (internal/process/next_tick.js:80:11)
process._tickCallback (internal/process/next_tick.js:104:9)
./runtests: line 114: 79569 Illegal Instruction     (core dumped) PATH=${NODE_INSTALL}/bin:${PATH} ${NODE_INSTALL}/bin/node --abort_on_uncaught_exception ${t}
     79570 Done                    | tee -a ${OUTPUT_DIR}/cloudapi.tap

Separately, on my COAL instance, this test in machines.test.js is failing:

not ok 462 should be equal
  ---
    operator: equal
    expected: 1
    actual:   2
    at: parseResponse (/opt/smartdc/cloudapi/node_modules/restify/lib/clients/json_client.js:91:9)
    stack: |-
      Error: should be equal
          at Test.assert [as _assert] (/opt/smartdc/cloudapi/node_modules/tape/lib/test.js:228:54)
          at Test.bound [as _assert] (/opt/smartdc/cloudapi/node_modules/tape/lib/test.js:80:32)
          at Test.equal (/opt/smartdc/cloudapi/node_modules/tape/lib/test.js:389:10)
          at Test.bound (/opt/smartdc/cloudapi/node_modules/tape/lib/test.js:80:32)
          at /opt/smartdc/cloudapi/test/machines/resize.js:45:19
          at parseResponse (/opt/smartdc/cloudapi/node_modules/restify/lib/clients/json_client.js:91:9)
          at IncomingMessage.done (/opt/smartdc/cloudapi/node_modules/restify/lib/clients/string_client.js:167:13)
          at IncomingMessage.g (events.js:292:16)
          at emitNone (events.js:91:20)
          at IncomingMessage.emit (events.js:185:7)
  ...

This is because two constraints (cpu and quota) are violated, not just one (quota):

{ code: 'ValidationFailed',
  message: 'Invalid VM update parameters',
  errors:
   [ { field: 'ram',
       code: 'InsufficientCapacity',
       message: 'Required additional RAM (10239744) exceeds the server\'s available RAM (-34953)' },
     { field: 'quota',
       code: 'InsufficientCapacity',
       message: 'Required additional disk (99990) exceeds the server\'s available disk (-1015)' } ] }

I've extended the test so it handles this case as well.

Various other minor fixes to get tests passing.

make check does not pass due to copyright check, but since I work for Spearhead Systems, I don't think my changing Joyent copyright headers is appropriate.

danmcd commented 3 years ago

The switch from @smaller/tap to tape happened with commit 26304bde0d77211f03a48972f46e55d9f00a38fa by @kusor . I wish to make sure the deepEqual behavior that concerns you isn't actually an improvement. My own JS knowledge is less than the two people I've tagged as reviewers, so I trust their opinions on this.

bahamat commented 3 years ago

@marsell Hey, thanks for doing this! I'm in favor of these changes, but I'd like to do the following: