wporg-code-analysis icon indicating copy to clipboard operation
wporg-code-analysis copied to clipboard

Clarify unit tests

Open iandunn opened this issue 4 years ago • 3 comments

The tests are kind time-consuming to grok. Here are some ideas to improve that:

Descriptive names

-function insecure_wpdb_query_1( $foo ) 
+function unescaped_concatenation( $foo )

-function insecure_wpdb_query_2( $foo )
+function unescaped_interpolation( $foo )

The "insecure wpdb query" part is obvious from the context, so omitting would make it easier to focus on what's unique about that test case.

Individual tests and data providers

All of the test classes only have two tests: test_unsafe_code and test_safe_code. That's unconventional from a PHPUnit perspective, and introduces a layer of abstraction that doesn't feel necessary. We also hardcode line numbers, which isn't descriptive either.

$this->assertEquals( $expected, $foundErrors[ 328 ][9][0][ 'message' ] );

A more straight-forward way might be to create a unique test function for each payload:

function test_concatenated_variable() {
    $payload = <<<PAYLOAD
    $wpdb->query( "SELECT * FROM $wpdb->users WHERE foo = '" . $foo . "' LIMIT 1" ); // unsafe
PAYLOAD

    $expected = 'WordPressDotOrg.sniffs.DirectDB.UnescapedDBParameter Unescaped parameter $foo used in $wpdb->query';
    $actual   =  $phpcs->processString( $payload ); // just psuedocode, not sure if a function like that exists out of the box

    $this->assertSame( $expected, $actual ); // could also use `expectOutputString()`, `expectException()`, etc, depending on how PHPCS provides the result.
}

In cases where there are lots of similar payloads for a type of test, data providers can keep them organized, and give good error messages when a specific case triggers an error.

If PHPCS requires something a file, then maybe we could simulate that with an IO stream?

Putting things in a file is consistent w/ how WPCS does their tests, though. @jrfnl, do you feel like that's best? If so, do you have any suggestions for improving clarity, etc?

iandunn avatar Apr 26 '21 19:04 iandunn

You are reinventing the wheel and no, I don't have time to spend coaching some experiment outside of all regular channels on how to do this.

jrfnl avatar Apr 26 '21 19:04 jrfnl

I'm sorry to hear you feel that way, but I'll stop asking for your input.

If you change your mind in the future, please let me know.

iandunn avatar Apr 26 '21 21:04 iandunn

$actual = $phpcs->processString( $payload ); // just psuedocode, not sure if a function like that exists out of the box

Looks like we might be able to use PHP_CodeSniffer\Files\DummyFile there. Haven't tested yet, though.

iandunn avatar Apr 26 '21 22:04 iandunn