PHPC-1113: Migrate tests to use a common URI env var #787

jmikola · 2018-03-23T21:19:44Z

https://jira.mongodb.org/browse/PHPC-1113

jmikola · 2018-03-23T21:21:38Z

tests/utils/basic-skipif.inc


+register_shutdown_function(function() {


See: http://php.net/manual/en/function.set-error-handler.php#112291

At the moment, I'm using all of these handlers to catch errors during SKIPIF refactoring. We can decide if they're worth keeping later on. It may be preferable to ignore SKIPIF errors/exceptions entirely and allow a test to fail outright.

jmikola · 2018-03-23T21:23:01Z

tests/utils/basic.inc

-    "NS"             => DATABASE_NAME . "." . COLLECTION_NAME,
-);
-def($consts);
+define('URI', getenv('MONGODB_URI') ?: 'mongodb://127.0.0.1/');


This is going to require changes to make test, which I intend to document in CONTRIBUTING.md. The C driver takes a similar approach: https://github.com/mongodb/mongo-c-driver/blob/master/CONTRIBUTING.md#testing

jmikola · 2018-03-23T21:24:30Z

tests/utils/tools.php

+ * @param integer $type
+ * @return string
+ */
+function server_type_as_string($type)


I'm aware that this function isn't currently used. I created it when the topology-based skip functions were producing more verbose messages (e.g. "we need a replica set but found a standalone"). If I don't find a use for it later, I'll delete this before merging.

I'd like to leave this in for now, as it may be useful down the line.

jmikola · 2018-03-23T21:25:26Z

tests/utils/tools.php

+ * @param integer $errno
+ * @param string
+ */
+function errno_as_string($errno)


Oh how I wish this was a built-in PHP function. Likewise for translating JSON constants.

jmikola · 2018-05-01T20:41:07Z

Status Update

I ran the test suite against standalone, standalone-auth, standalone-ssl, and replica set topologies by setting the MONGODB_URI environment variable to one of the URIs in make list-servers and running make test. The standalone and standalone-auth environments pass completely. The standalone-ssl environment required manually adding "/?ssl=true" to the URI but failed due to self-signed certificates (PHPC-1180). The replica set topology had the following failures, which I'll look into next:

Causal consistency: $clusterTime is not sent in commands to unsupported deployments [tests/causal-consistency/causal-consistency-011.phpt]
MongoDB\Driver\Manager::executeCommand() [tests/manager/manager-executeCommand-001.phpt]
MongoDB\Driver\Manager::executeWriteCommand() throws CommandException for unsupported update operator [tests/manager/manager-executeWriteCommand_error-003.phpt]
MongoDB\Driver\Manager::getServers() (standalone) [tests/manager/manager-getservers-001.phpt]
MongoDB\Driver\Manager: Constructing invalid manager [tests/manager/manager-var-dump-001.phpt]
PHPC-671: Segfault if Manager is already freed when using selected Server [tests/server/bug0671-002.phpt]
MongoDB\Driver\Server debugInfo [tests/server/server-debug.phpt]
MongoDB\Driver\Server::executeCommand() [tests/server/server-executeCommand-001.phpt]
MongoDB\Driver\Server::executeReadWriteCommand() [tests/server/server-executeReadWriteCommand-001.phpt]
MongoDB\Driver\Server::executeWriteCommand() [tests/server/server-executeWriteCommand-001.phpt]
MongoDB\Driver\Server::getTags() with standalone [tests/server/server-getTags-001.phpt]
MongoDB\Driver\Manager::executeInsert() [tests/standalone/write-error-001.phpt]
PHPC-671: Segfault if Manager is already freed when using WriteResult's Server [tests/writeResult/bug0671-003.phpt]

These are likely related to the expected output making assumptions about running on a standalone.

This sentence has been dangling since ae0219d.

This removes previous assumptions about the structure of the URI string.

Some of these calls are NOPs and can be removed, while others should use drop_collection().

jmikola · 2018-05-02T22:57:45Z

Tests should now be passing a replica set topology. I believe all that's left will be to add additional topologies to Travis, which I can handle in another ticket.

@derickr: Please review.

derickr

Looks largely fine. I have some questions, and I would prefer the deferred tests to be XFAILs, but I'm not too hung up about that.

derickr · 2018-05-03T10:56:01Z

tests/cursor/cursor-IteratorIterator-004.phpt

@@ -2,7 +2,8 @@
 MongoDB\Driver\Cursor iteration beyond last document (OP_QUERY)
 --SKIPIF--
 <?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
-<?php NEEDS('STANDALONE_30'); ?>
+<?php skip_if_not_live(); ?>
+<?php skip_if_server_version('>', '3.0.99'); ?>


Don't want to make these '>=', '3.1' ?

That's technically cleaner. I'll do so.

derickr · 2018-05-03T10:57:00Z

tests/connect/standalone-plain-0001.phpt

@@ -1,8 +1,8 @@
 --TEST--
 Connect to MongoDB with using PLAIN auth mechanism
 --SKIPIF--
+<?php echo "skip authMechanism=PLAIN (LDAP) tests must be reimplemented (PHPC-1172)\n"; ?>


I prefer having these as XFAIL for now, and not just skipped.

Will make the change across the board for these.

derickr · 2018-05-03T10:58:00Z

tests/connect/replicaset-seedlist-001.phpt

@@ -1,6 +1,7 @@
 --TEST--
 MongoDB\Driver\Manager: Connecting to Replica Set with only secondary in seedlist
 --SKIPIF--
+<?php echo "skip replica set seedlist tests must be reimplemented (PHPC-1173)\n"; ?>


These as XFAIL too please.

derickr · 2018-05-03T10:58:31Z

tests/apm/bug0950-001.phpt

@@ -8,7 +8,7 @@ PHPC-950: Segfault killing cursor after subscriber HashTable is destroyed (no su
 <?php
 require_once __DIR__ . "/../utils/basic.inc";

-$manager = new MongoDB\Driver\Manager(STANDALONE);
+$manager = new MongoDB\Driver\Manager(URI);


You did write a script for this one, right? :-)

This was a scripted search and replace, but everything else was manual as it required adding c conditional skips (e.g. REPLICASET needed to require a replica set topology). After bulk-changing these to URI, I then ran the suite against various topologies to determine which tests were not topology-agnostic.

derickr · 2018-05-03T10:59:53Z

tests/connect/standalone-auth-0002.phpt

@@ -1,6 +1,7 @@
 --TEST--
 Connect to MongoDB with using default auth mechanism #002
 --SKIPIF--
+<?php echo "skip parse_url() tests must be reimplemented (PHPC-1177)\n"; ?>


XFAIL again, please.

derickr · 2018-05-03T11:00:07Z

tests/functional/cursor-001.phpt

@@ -1,6 +1,7 @@
 --TEST--
 Sorting single field, ascending, using the Cursor Iterator
 --SKIPIF--
+<?php echo "skip LOAD() tests must be reimplemented (PHPC-1178)\n"; ?>


XFAIL again, please.

derickr · 2018-05-03T11:00:46Z

tests/apm/monitoring-addSubscriber-001.phpt

@@ -26,7 +26,6 @@ class MySubscriber implements MongoDB\Driver\Monitoring\CommandSubscriber
    }
 }

-CLEANUP( STANDALONE );


Does this not mean we're not cleaning up data? Maybe I misunderstand.

These instances of CLEANUP() were redundant, as it was already done in SKIPIF. See:

mongo-php-driver/tests/apm/monitoring-addSubscriber-001.phpt

Line 4 in 0273c79

<?php require __DIR__ . "/../utils/basic-skipif.inc"; CLEANUP(STANDALONE) ?>

derickr · 2018-05-03T11:00:57Z

tests/bulk/bulkwrite-update-arrayFilters-001.phpt

@@ -1,6 +1,7 @@
 --TEST--
 MongoDB\Driver\BulkWrite::update with arrayFilters
 --SKIPIF--
+<?php echo "skip START() tests must be reimplemented (PHPC-1179)\n"; ?>


XFAIl, please.

derickr · 2018-05-03T11:02:10Z

tests/server/server-executeReadWriteCommand-001.phpt

@@ -11,7 +11,7 @@ require_once __DIR__ . "/../utils/basic.inc";
 require_once __DIR__ . "/../utils/observer.php";

 $manager = new MongoDB\Driver\Manager(URI);
-$server = $manager->selectServer(new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_SECONDARY));
+$server = $manager->selectServer(new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_PRIMARY));


There must be a reason why RP_SECONDARY worked before — do you know why that is?

If the topology type is single (i.e. standalone), mongoc_topology_description_select() ignores the read preference. This conforms to the server selection spec's rules for Topology type: Single.

Left in, this raises a warning which results in skipping the test, which defeats the point of XFAIL.

derickr

LGTM

jmikola commented Mar 23, 2018

View reviewed changes

jmikola force-pushed the phpc-1113 branch 4 times, most recently from 46a4273 to 2e80266 Compare March 28, 2018 21:17

jmikola force-pushed the phpc-1113 branch 3 times, most recently from 897329d to a6a4e62 Compare May 1, 2018 20:12

jmikola added 21 commits May 2, 2018 16:04

PHPC-1113: Use common URI env var in basic/skipif includes

f4cf0fe

PHPC-1113: Replace NEEDS_CRYPTO() with skip function

8a83606

PHPC-1113: Replace NEEDS_SSL() with skip function

cf6aa41

PHPC-1113: Replace NEEDS_STORAGE_ENGINE() with skip function

6960868

PHPC-1113: Replace NEEDS_ATLEAST_MONGODB_VERSION() with skip function

fe52b48

PHPC-1113: Replace TESTCOMMANDS() with skip function

38405a7

PHPC-1113: Replace NEEDS('REPLICASET') with skip function

3a1362b

PHPC-1113: Replace NEEDS('STANDALONE_AUTH') with skip function

8768f54

PHPC-1113: Replace NEEDS('STANDALONE_X509') with skip function

55e3341

PHPC-1113: Replace CLEANUP() with skip function

b4062f8

PHPC-1113: Replace NEEDS('STANDALONE_SSL') with skip function

fc4cbd4

PHPC-1113: Replace NEEDS('STANDALONE') with skip function

5f7f350

PHPC-1113: Replace NEEDS('STANDALONE_30') with skip functions

037e393

PHPC-1113: Replace NEEDS('REPLICASET_30') with skip functions

9d4bc8d

PHPC-1113: Disable LDAP tests (defer to PHPC-1172)

f242f9d

PHPC-1113: Disable replica set seedlist tests (defer to PHPC-1173)

0534486

PHPC-1113: Remove NEEDS() from manual DNS seedlist test

e50a011

Update comment and title in SSL tests

9f7d2af

PHPC-1113: Require SSL for MONGODB-X509 tests

0f15a16

PHPC-1113: Use common URI constant

58fa85e

Test for PHPC-1045 should require that the URI has no username

2cbe426

jmikola added 10 commits May 2, 2018 16:04

Finish sentence in test comment

73fc8a5

This sentence has been dangling since ae0219d.

Create append_uri_option() helper function

014f2b6

This removes previous assumptions about the structure of the URI string.

PHPC-1113: Disable parse_url() tests (defer to PHPC-1177)

7ddb1fd

PHPC-1113: Disable tests that load data fixtures (defer to PHPC-1178)

25f9c3d

PHPC-1113: Replace CLEANUP() calls in APM tests

aee1d13

Some of these calls are NOPs and can be removed, while others should use drop_collection().

PHPC-1113: Disable tests that start servers with MO (defer to PHPC-1179)

27a0452

PHPC-1113: Use skip_if_not_standalone() for tests requiring a standalone

6cafb28

Fix test titles

f79a626

PHPC-1113: Allow extra output for replica sets

fc8b862

Select primary server for use in write command tests

f7d8169

jmikola force-pushed the phpc-1113 branch from a6a4e62 to f7d8169 Compare May 2, 2018 20:45

PHPC-1113: Make WriteError tests server version agnostic

62797b0

jmikola requested a review from derickr May 2, 2018 22:57

jmikola changed the title ~~[WIP] PHPC-1113: Migrate tests to use a common URI env var~~ PHPC-1113: Migrate tests to use a common URI env var May 2, 2018

derickr reviewed May 3, 2018

View reviewed changes

jmikola added 3 commits May 3, 2018 11:37

PHPC-1113: Use >=3.1 for version skips instead of >3.0.99

8ca94d0

PHPC-1113: Use XFAIL instead of skips for tests to be reimplemented

dcc2fdf

PHPC-1113: Remove LOAD(STANDALONE) from tests to be reimplemented

e70c79d

Left in, this raises a warning which results in skipping the test, which defeats the point of XFAIL.

derickr approved these changes May 3, 2018

View reviewed changes

jmikola merged commit e70c79d into mongodb:master May 3, 2018

jmikola added a commit that referenced this pull request May 3, 2018

Merge pull request #787

1c13b7f

jmikola deleted the phpc-1113 branch May 3, 2018 17:20

jmikola mentioned this pull request May 25, 2018

PHPC-1195: BulkWrite::update() should append arrayFilters as array #842

Merged


		register_shutdown_function(function() {

PHPC-1113: Migrate tests to use a common URI env var #787

PHPC-1113: Migrate tests to use a common URI env var #787

Uh oh!

Conversation

jmikola commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola commented May 1, 2018

Status Update

Uh oh!

jmikola commented May 2, 2018

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!