MANTA-4341 Optimize conditional object operations #382
Conversation
There was a problem hiding this comment.
I'm seeing these 3 test failures when I run the buckets tests:
[kelly@mantadev node-manta]$ make test TEST_FILTER=buckets
test/integration/buckets-client-basic.test.js ....... 38/38
test/integration/buckets-client-condreq.test.js ... 106/109
CreateBucketObject: if-match (bad)
not ok should be equal
--- wanted
+++ found
-false
+[null]
compare: ===
at:
line: 403
column: 15
file: test/integration/buckets-client-condreq.test.js
stack: |
test/integration/buckets-client-condreq.test.js:403:15
IncomingMessage.onEnd (lib/buckets.js:270:9)
source: |
t.equal(inStream.readableEnded, false);
CreateBucketObject: if-none-match (bad)
not ok should be equal
--- wanted
+++ found
-false
+[null]
compare: ===
at:
line: 428
column: 15
file: test/integration/buckets-client-condreq.test.js
stack: |
test/integration/buckets-client-condreq.test.js:428:15
IncomingMessage.onEnd (lib/buckets.js:270:9)
source: |
t.equal(inStream.readableEnded, false);
CreateBucketObject: if-match (good)
not ok should be equal
--- wanted
+++ found
-true
+[null]
compare: ===
at:
line: 452
column: 15
file: test/integration/buckets-client-condreq.test.js
stack: |
test/integration/buckets-client-condreq.test.js:452:15
IncomingMessage.onEnd (lib/buckets.js:1089:29)
source: |
t.equal(inStream.readableEnded, true);
test/integration/buckets-client-validation.test.js .. 78/78
test/unit/buckets-mantauri.test.js .................. 32/32
test/unit/buckets-mkBucketReqOpts.test.js ........... 14/14
total ............................................. 268/271
268 passing (12s)
3 failing
make: *** [Makefile:62: test] Error 1
Have you seen these? I'll try to dig in more and see if I can understand what is going on. I tried using both node v6 and v8 to run the tests and I see the same result with both.
|
Ah, looks like that property is only available from Node v13 (nodejs/node#28814). I'll see if there's a better way to verify this behaviour in the test suite. |
…ts was low anyway.
|
So we can't get at these properties in earlier versions of node unless we reach into I haven't put a great deal of thought into these tests so there's a good chance they're not testing the right thing anyway. I've removed them in this latest push, but I'd be happy to revisit if you think otherwise! |
I think it's probably ok to leave them out versus reaching into |
| t.ok(err); | ||
| t.ok(res); | ||
| t.equal(res.statusCode, 400); | ||
| //console.log(err); |
| } | ||
| }, | ||
| function (err, res) { | ||
| //t.ok(err); |
| t.ok(err); | ||
| t.ok(res); | ||
| /* | ||
| * XXX Why no `res.statusCode` from client, like `get` does. |
There was a problem hiding this comment.
This is really odd. Wonder why it would be different in this case?
There was a problem hiding this comment.
Oh man, sorry about this one. I think my brain is a bit frazzled. I just spent way too long looking at why node-manta wasn't parsing a response body from a HEAD request...
So the difference here is that node-manta's buckets client does a lot of the response wrangling itself, but for GETs it leans on the existing directory client for some niceties around resumability.
The difference is that the buckets error path doesn't return res on error and instead populates err with all the information. Directories is different where res is included as well it seems, so this is why GET in the buckets client also includes res.
The latest commit should account for this in the test suite. I think there is still some work to do to make this consistent across all client requests, but it's not a million miles off and I expect explains why the buckets work is still on its own branch.
These are the tests I've been using the check my work on MANTA-4341. It's not totally clean yet, and I'm still not sure about things like the lack of
res.statusCodeon a PreconditionFailedError.I wanted to wrap it up into a PR though because the other parts of MANTA-4341 are due to be reviewed and it'll be good to at least prove there's been some testing. Perhaps it'll also be good to get some early review as well to make sure it's not a million miles off.