test-related questions, understand whether you need to change the test in the testMcryptGenericMode

[jhonatasfender]

Well I noticed that in the tests has a "Risky", and I went to look at the code:

    /**
     * @dataProvider mcryptBlockModuleNameProvider
     */
    public function testMcryptGenericMode($modeName, $validMode)
    {
        if (!$validMode) {
            return;
        }
        $key = str_repeat('a', 16);
        $iv = str_repeat('b', 16);
        $plaintext = str_repeat('c', 16);
        $mcrypt = mcrypt_encrypt('rijndael-128', $key, $plaintext, $modeName, $iv);
        $compat = phpseclib_mcrypt_encrypt('rijndael-128', $key, $plaintext, $modeName, $iv);
        $this->assertEquals(bin2hex($mcrypt), bin2hex($compat));
    }

And you wanted to know if it's for you this way, or do you accept that you make changes to this method?

I ran the tests on my machine, and this was the displayed log of script execution:

$ ./vendor/bin/phpunit
PHPUnit 6.5.10 by Sebastian Bergmann and contributors.

.....................S.S.S.....................................  63 / 132 ( 47%)
.............SSS.S.SSS.SSS.SSS.S.S.S.S.S.S.S.S.S.S.S.S.S...SS.. 126 / 132 ( 95%)
.....R                                                          132 / 132 (100%)

Time: 15.28 minutes, Memory: 6.00MB

There was 1 risky test:

1) MCryptCompatTest::testMcryptGenericMode with data set #6 ('invalid-mode', false)
This test did not perform any assertions

OK, but incomplete, skipped, or risky tests!
Tests: 132, Assertions: 186, Skipped: 31, Risky: 1.

[terrafrost]

First, you should squash your commits into a single commit.

Second, you shouldn't include a composer.lock file. That's more intended for projects - not libraries. https://blog.martinhujer.cz/17-tips-for-using-composer-efficiently/ elaborates.

I'll make a few other comments in line.

[terrafrost]

Well, actually, I'm not sure in-line comments are necessary.

I'm not sure your README.md changes make a lot of sense. You say, in a newly added "How to Contribute" section, simply "composer install". That's doesn't really tell me how to contribute. And the new "Tests" section... idk it seems a bit redundant to me. ./vendor/bin/phpunit is pretty standard...

You did identify a few areas that could benefit from coding standards cleanup. eg. changing case'tripledes': to case 'tripledes': is an obvious improvement. But some of the other changes are more subjective in nature. eg. changing this:

return $mode[0] == 'N' ?
    'n' . substr($mode, 1) :
    $mode;

...to this:

return $mode[0] == 'N' ? 'n' . substr($mode, 1) : $mode;

And for the unit tests... you changed the data provider for testMcryptGenericMode to a new method - mcryptBlockModuleNameWithoutInvalidModuleProvider. If we're gonna have a new method, however, then why pass two parameters? Why not just pass one? In this new method every mode is valid so there's no need to pass the valid status to testMcryptGenericMode...

[jhonatasfender]

Thank you very much for the comments, I will make the necessary changes, and remove those redundant ones.

In the test question, I created a new method to solve a problem that I was presenting as a risk, and then I detected that I had an invalid-mode option with a false value, and then I immediately followed a validation and returned, where I decided to create a new provider with different values.

Thank you very much in advance.