Query arguments are not stored or verified

[avgeorge]

The arguments to a query are not stored when creating a recording, and not verified when playing back recording (obviously because there is nothing to verify with). This means that it is possible to change the arguments to a query without failing existing tests, as long as the sequence of queries does not change.
As an example, consider this line in the simple query test:

copyist/drivertest/commontest/common.go

Line 115 in 0d1c754

rows, err := db.Query("SELECT name FROM customers WHERE id=$1", 1)

If we change the id argument to 2 or 50 the result should change to a different name or no rows in the result set respectively. However running the test with playback (I used pqtest but I think it should be the same for all) results in a pass with no warning that the tests need to recorded again. If you update the test to look for the correct name or error then the test fails and gives a message suggesting you might need to redo the recording.
I know this example is a bit silly because you are changing the test without changing the expected result or redoing the recording, but the same thing could happen if you make changes in your application logic which ends up affecting the arguments to a query.

Is there a reason to not include the query arguments in the recording? I don't think serialization should be an issue because the library already handles that for the results.

[andy-kimball]

This behavior is deliberate. There are many important cases where the parameters need to change without requiring a re-recording. The most common is that ORMs like Pop will automatically generate UUID id column values and timestamps for created_at columns. If copyist verified those values, it would always fail, since these are different every time.

You might ask, "but isn't that a problem that these are changing each time the app runs?". The answer is that it almost never matters, because most apps do not read these columns from the original objects that were written. Although non-deterministic operations can cause copyist problems (and UUIDs and timestamps are inherently non-deterministic), as long as the app does not observe the side-effects, then there's no issue. And in the few cases where it does observe them, it's usually possible to work around the side-effect by reloading the object from the database (which allows copyist to repopulate the object with the originally recorded values).

Hopefully that explanation makes sense - it originally took me some time to realize how important it was to not verify input parameters. At Cockroach Labs, we heavily use copyist, and once in a while this behavior does affect us - we end up with a test that runs fine during playback, but fails during recording. We've talked about running all tests in recording mode in our nightly test runs to catch this, but it's just never been a big enough problem to justify that work.

[avgeorge]

Thanks, you've made it clear why recording the inputs would be undesirable in some cases. Still, if we knew that our inputs were deterministic could we opt in to recording the inputs as well?

[andy-kimball]

Yeah, I think it'd be possible to have a mode where copyist checks inputs (probably a command-line flag and/or global method to enable this mode). If you or another interested community member wants to submit a PR to add this, I'd be happy to accept it, assuming it's nice, neat code that fits in with the existing ways of doing things in copyist.