Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify unit tests #50

Open
iandunn opened this issue Apr 26, 2021 · 3 comments
Open

Clarify unit tests #50

iandunn opened this issue Apr 26, 2021 · 3 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@iandunn
Copy link
Member

iandunn commented Apr 26, 2021

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?

@jrfnl
Copy link
Member

jrfnl commented Apr 26, 2021

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.

@iandunn
Copy link
Member Author

iandunn commented Apr 26, 2021

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 iandunn added enhancement New feature or request question Further information is requested labels Apr 26, 2021
@iandunn iandunn added this to the 1: Create Plugin Standard milestone Apr 26, 2021
@iandunn
Copy link
Member Author

iandunn commented Apr 26, 2021

$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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants