PHPLIB-981: Add tests for examples #1002

alcaeus · 2022-11-14T07:50:30Z

This PR adds tests for the example scripts. To ensure good coverage, the tests attempt to match against the output of the example scripts. This requires a change in the examples to no longer output to STDERR, as PHPUnit considers any output to STDERR as condition to fail a test and doesn't allow asserting against it.

When running the tests, I noticed that the tests fail on sharded clusters, because the command replies are different. This would complicate the output matchers by quite a lot, so I'm wondering if it's actually worth matching against the actual command output, or if we're happy asserting that the rough information is the same.

I will likely also change the examples for type maps and persistable classes to no longer use var_dump. Having xdebug installed (which is likely on dev machines) produces different output for var_dump, making the tests fail on developer machines.

alcaeus · 2022-11-14T07:51:28Z

tests/ExamplesTest.php

+
+        $expectedOutput = <<<'OUTPUT'
+drop command started
+command: { "drop" : "coll", "$db" : "test", "lsid" : { %s }%S }


This command output here and all subsequent command documents could be replaced with { %s } if we're not interested in asserting the actual command results.

I don't see any reason to assert the command contents. Just use { %s } for everything, or rather: command: %s. I think the only point of testing these was to ensure they actually run with no PHP errors or uncaught exceptions. We don't need the same level of scrutiny as the doc snippets that go in the MongoDB manual.

alcaeus · 2022-11-14T07:53:00Z

tests/ExamplesTest.php

+        ];
+    }
+
+    public function testChangeStream(): void


This test (and testWithTransaction) below was extracted as it needs to check whether change streams are supported. I initially wanted to provide a closure in the data provider, but that causes errors due to closures not being serialisable (which is required to run tests in isolation). Instead of passing a string callable I opted for extracting the tests from the data provider and calling the original test method manually instead.

but that causes errors due to closures not being serialisable

Looks like a job for jeremeamia/super_closure! 🦸

Since deprecated for opis/closure. I'm not actually suggesting this.

alcaeus · 2022-11-14T07:53:36Z

examples/changestream.php

-        fprintf(STDERR, "Aborting after 3 seconds...\n");
+        printf("Aborting after 3 seconds...\n");


Necessary to avoid PHPUnit considering the tests failed due to output, and to allow asserting against the full test output.

jmikola · 2022-11-14T13:16:31Z

tests/ExamplesTest.php

+
+        $expectedOutput = <<<'OUTPUT'
+drop command started
+command: { "drop" : "coll", "$db" : "test", "lsid" : { %s }%S }


I don't see any reason to assert the command contents. Just use { %s } for everything, or rather: command: %s. I think the only point of testing these was to ensure they actually run with no PHP errors or uncaught exceptions. We don't need the same level of scrutiny as the doc snippets that go in the MongoDB manual.

jmikola · 2022-11-14T13:19:00Z

tests/ExamplesTest.php

+        ];
+    }
+
+    public function testChangeStream(): void


but that causes errors due to closures not being serialisable

Looks like a job for jeremeamia/super_closure! 🦸

Since deprecated for opis/closure. I'm not actually suggesting this.

jmikola · 2022-11-14T15:47:26Z

tests/ExamplesTest.php

+{ "_id" : { "_data" : "%s" }, "operationType" : "insert", "clusterTime" : { "$timestamp" : { "t" : %d, "i" : %d } }%S, "fullDocument" : { "_id" : { "$oid" : "%s" }, "x" : 6 }, "ns" : { "db" : "test", "coll" : "coll" }, "documentKey" : { "_id" : { "$oid" : "%s" } } }
+{ "_id" : { "_data" : "%s" }, "operationType" : "insert", "clusterTime" : { "$timestamp" : { "t" : %d, "i" : %d } }%S, "fullDocument" : { "_id" : { "$oid" : "%s" }, "x" : 7 }, "ns" : { "db" : "test", "coll" : "coll" }, "documentKey" : { "_id" : { "$oid" : "%s" } } }
+{ "_id" : { "_data" : "%s" }, "operationType" : "insert", "clusterTime" : { "$timestamp" : { "t" : %d, "i" : %d } }%S, "fullDocument" : { "_id" : { "$oid" : "%s" }, "x" : 8 }, "ns" : { "db" : "test", "coll" : "coll" }, "documentKey" : { "_id" : { "$oid" : "%s" } } }
+{ "_id" : { "_data" : "%s" }, "operationType" : "insert", "clusterTime" : { "$timestamp" : { "t" : %d, "i" : %d } }%S, "fullDocument" : { "_id" : { "$oid" : "%s" }, "x" : 9 }, "ns" : { "db" : "test", "coll" : "coll" }, "documentKey" : { "_id" : { "$oid" : "%s" } } }


This seems fragile should the server introduce new response fields to change events. I'd suggest changing this to { %s } assertions for each line. You can also lower the number of inserts in the example (maybe 5 instead of 10) if you want to make this more concise. There's really no reason it needs to emit this much output.

…oDB <= 4.2

On sharded cluster, collection deletion is sometimes not propagated across the cluster, leading to different output even if we drop the collection before the test.

This should help reduce test failures

jmikola · 2022-11-22T03:08:20Z

tests/ExamplesTest.php

@@ -11,6 +11,10 @@ public function setUp(): void
    {
        parent::setUp();

+        if ($this->isShardedCluster()) {
+            $this->markTestSkipped('Examples are not tested on sharded clusters.');
+        }


Obligatory @amschwerin:

I am a creature of habit: mongodb/specifications#565 (comment)

I tried hard to keep those tests in, but unfortunately some tests failed on sharing due to a server error ("no progress was made executing batch op after 5 rounds"). Since we're only dealing with examples here I opted against spending more time on testing against sharded clusters and gave up trying.

alcaeus · 2022-11-22T07:40:21Z

Note: test failures against MongoDB latest tracked in PHPLIB-1044.

jmikola

Some optional suggestions to make the test collections consistent with their filename and consider a property hint instead of calling assert(). Everything else looks fine as-is.

psalm-baseline.xml

jmikola · 2022-11-22T08:29:23Z

src/Operation/BulkWrite.php

@@ -325,6 +326,8 @@ public function execute(Server $server)
            $type = key($operation);
            $args = current($operation);

+            assert(is_array($args));


Is this necessary? Since $operations is private couldn't you also just add an array property type hint to it?

I believe the error crept up due to a change in the baseline, so I've removed this assert call and updated the baseline instead. Fixing this would've required updating types in more than just this file, which is why it was initially deferred. We have follow-up tickets to fix issues like this one.

examples/command_logger.php

examples/with_transaction.php

Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

Fixing this requires a larger-scale refactoring of the types involved, which was deferred to a later date.

alcaeus added 2 commits November 10, 2022 11:45

No longer print to STDERR

b065dd1

PHPLIB-981: Test example scripts

1201c1a

alcaeus commented Nov 14, 2022

View reviewed changes

alcaeus changed the title ~~PHPLIB-891: Add tests for examples~~ PHPLIB-981: Add tests for examples Nov 14, 2022

jmikola reviewed Nov 14, 2022

View reviewed changes

Relax command/reply assertions

6f8e522

alcaeus force-pushed the phplib-891-test-examples branch 2 times, most recently from c6f6ef1 to bd9d8d7 Compare November 15, 2022 09:48

alcaeus added 3 commits November 15, 2022 11:46

Create test collection in transaction example to avoid errors on Mong…

e3a4433

…oDB <= 4.2

Update command logger test output to handle collection differences

0f94fe0

On sharded cluster, collection deletion is sometimes not propagated across the cluster, leading to different output even if we drop the collection before the test.

Create test collection in change stream example

64583c7

alcaeus force-pushed the phplib-891-test-examples branch 2 times, most recently from e4161ce to 267535a Compare November 21, 2022 10:29

alcaeus added 3 commits November 21, 2022 12:21

Use majority write concern in bulk example

8404531

Fix psalm errors

a3246d1

Use different collection names in each example

b58811c

This should help reduce test failures

alcaeus force-pushed the phplib-891-test-examples branch from 267535a to b58811c Compare November 21, 2022 11:22

Skip example tests on sharded clusters

4d99671

jmikola reviewed Nov 22, 2022

View reviewed changes

Update psalm-baseline on PHP 7.4

834dbf8

alcaeus marked this pull request as ready for review November 22, 2022 07:39

alcaeus requested a review from jmikola November 22, 2022 07:39

jmikola approved these changes Nov 22, 2022

View reviewed changes

alcaeus and others added 2 commits November 22, 2022 09:57

Update collection names in tests

8b134fd

Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

Exclude bulk write psalm error

f60243b

Fixing this requires a larger-scale refactoring of the types involved, which was deferred to a later date.

alcaeus force-pushed the phplib-891-test-examples branch from 4cbe45a to f60243b Compare November 22, 2022 08:58

alcaeus merged commit b309cf8 into mongodb:master Nov 22, 2022

alcaeus deleted the phplib-891-test-examples branch November 22, 2022 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPLIB-981: Add tests for examples #1002

PHPLIB-981: Add tests for examples #1002

alcaeus commented Nov 14, 2022 •

edited

Loading

alcaeus Nov 14, 2022

jmikola Nov 14, 2022

alcaeus Nov 14, 2022

jmikola Nov 14, 2022

alcaeus Nov 14, 2022

jmikola Nov 14, 2022

jmikola Nov 14, 2022

jmikola Nov 14, 2022

jmikola Nov 22, 2022

alcaeus Nov 22, 2022

alcaeus commented Nov 22, 2022

jmikola left a comment •

edited

Loading

jmikola Nov 22, 2022

alcaeus Nov 22, 2022

		fprintf(STDERR, "Aborting after 3 seconds...\n");
		printf("Aborting after 3 seconds...\n");

PHPLIB-981: Add tests for examples #1002

PHPLIB-981: Add tests for examples #1002

Conversation

alcaeus commented Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus commented Nov 22, 2022

jmikola left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus commented Nov 14, 2022 •

edited

Loading

jmikola left a comment •

edited

Loading