PHPLIB-463: Implement spec test runner for retryable reads

[alcaeus]

https://jira.mongodb.org/browse/PHPLIB-463

[jmikola]

I understand why this was necessary, as libmongoc does not prohibit retrying mapReduce if executed via read_command_with_opts(). We'd be in a pickle of mapReduce supported a readConcern option, as this would prohibit inheritance of any URI-level read concern; however, it does not and I don't imagine that will change anytime soon.

I do think we'd do well to add a command above this line explaining why we use executeCommand() here, though. Feel free to note that this isn't a problem since MR doesn't take a read concern.

[alcaeus]

Please correct me if I'm wrong, but Server::executeCommand uses PHONGO_COMMAND_RAW (0x07). The way I read the readConcern check in phongo_execute_command, it will parse the read concern when using PHONGO_COMMAND_RAW.

However, when looking at mongoc-client-c, it only applies a readConcern option for MONGOC_CMD_READ and MONGOC_CMD_RW.

Looking at the enum we declare in phongo.h, I believe the declaration for PHONGO_COMMAND_RAW should change from 0x07 to 0x06 to not support the readConcern option, just the way libmongoc does.

In summary, your point is absolutely correct, as libmongoc does not parse a readConcern option when running the command through executeCommand vs. executeReadCommand. Given the history of mapReduce, I'd also be surprised if it started supporting a readConcern option. If that were the case however, there would be more changes necessary in both libmongoc and the driver here, so I think we should be safe ignoring this for now. Regardless of the above, I've added a comment to explain why we use executeCommand instead of executeReadCommand.

[jmikola]

I think we talked through the first question earlier today, but in case not: Server::executeCommand() takes RC, RP, and WC options, just as mongoc_client_command_with_opts() does. What those functions don't do is inherit client-level options from the URI.

That aside, the new comment LGTM.

[jmikola]

A couple of comments, but LGTM after those are acknowledged. I think the watch() assertion is the only thing that may need a correction.

[jmikola]

Did I suggest using ASSERT_SAME_DOCUMENTS in my previous review here? This would ensure that we start failing if/when the tests ever introduce result assertions for watch(). As-is, this will cause us to ignore newly introduced result assertions.

[alcaeus]

I missed that the original comment referred to match as well. It's updated now.

[jmikola]

The other execute methods in this file start with:

$args = $context->prepareOptions($this->arguments);
$context->replaceArgumentSessionPlaceholder($args);

I suppose there's no reason to do this because we reference exact options in the cases below and aren't passing the full array to anything. I suppose it's something to keep in mind if/when GridFS gets session support, although who knows what that API may look like.

[alcaeus]

I suppose there's no reason to do this because we reference exact options in the cases below and aren't passing the full array to anything. I suppose it's something to keep in mind if/when GridFS gets session support, although who knows what that API may look like.

That is correct. We only use the filename option for the download_by_name operation and leave all other options unused. We could pass in options on the off-chance that a test specifies the revision option (which currently is the only option taken by these GridFS operations).

[jmikola]

I think it's worth doing array_diff_key() just to populate general options to each method that takes them.

[alcaeus]

Merged in 2789277.