Skip to content

Unify structure for ext/random's engine tests #9321

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

Merged
merged 2 commits into from
Aug 23, 2022

Conversation

TimWolla
Copy link
Member

This PR makes a sweeping change across all tests in ext/random/tests/02_engine to achieve a uniform appearance:

  • Use a descriptive test title.
  • Import all classes with use to reduce the visual noise within the test for repeated references to specific classes.
  • Do not use the backslash in front of function calls for consistency with tests elsewhere.
  • Do not use anonymous classes to give each engine a descriptive name.
  • Use echo $e->getMessage(), PHP_EOL; with a comma to dump error messages.
  • Use var_dump() to output engine values.
  • Use 10_000 as the consistent number of verification iterations, instead of 1000, 5000 and 10000.
  • Remove the Xoshiro128++ userland engine, because it does not really test ext/random, but rather PHP's bit fiddling capabilities.

This test does not exercise any of the code paths of the random extension and
thus is no value-add.
$engines[] = new \Random\Engine\Mt19937(1234);
$engines[] = new \Random\Engine\PcgOneseq128XslRr64(1234);
$engines[] = new \Random\Engine\Xoshiro256StarStar(1234);
$engines[] = new Mt19937(1234);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of a form like the first PR, where the implemented class name is defined in a separate file and iterated over. e.g.) engines.inc.
https://github.com/zeriyoshi/php-src/blob/ed42ae2c2fe30ad9e608b6e693cd3ef3c60f1d85/ext/random/tests/number_generator/common.inc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this not as readable, because one needs to keep the contents of the include file in the head, instead of simply reading the test from top to bottom.

Copy link
Contributor

@zeriyoshi zeriyoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is in place, and you've properly corrected the sloppy test I wrote! Excellent!

Just one thing: the userland PRNG implementation probably needs to be tested. Other than this point, I think other is a trivial issue.

@zeriyoshi
Copy link
Contributor

(I admit, in all honesty, that my initial testing was messy. Repeated implementation changes made tracking cumbersome and messy. You should originally create appropriate tests before proceeding with the implementation. Please do not imitate me!)

@TimWolla
Copy link
Member Author

Just one thing: the userland PRNG implementation probably needs to be tested. Other than this point, I think other is a trivial issue.

As you already resolved the comment yourself: Is this still relevant? We could add it as a randomizer test, where the engine will actually need to interact with ext/random.

@zeriyoshi
Copy link
Contributor

zeriyoshi commented Aug 16, 2022

@TimWolla
I was not used to using GH and was operating it in a strange way...

How about using 64-bit PRNGs (PCG, Xoshiro) but adding a user-defined RNG as a test that forces truncation to 32-bit length?

<?php

use Random\Randomizer;
use Random\Engine;
use Random\Engine\Xoshiro256StarStar;

$randomizer = new Randomizer(
    new class implements Engine
    {
        private readonly Xoshiro256StarStar $engine;

        public function __construct()
        {
            $this->engine = new Xoshiro256StarStar();
        }

        public function generate(): string
        {
            return substr($this->engine->generate(), -4 /* char(8-bit) * 4 = 32-bit */);
        }
    }
);
// ~~~~

Copy link
Contributor

@zeriyoshi zeriyoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested changes may be better served by another Pull Request
LGTM

@TimWolla
Copy link
Member Author

@zeriyoshi I'm currently on vacation and will look into this again next week. It's not urgent anyway.

@TimWolla
Copy link
Member Author

As you approved, I'll merge this now. I'll create a follow-up PR for the Randomizer tests and add a real-world user-engine to those then (e.g. Xoshiro128++).

@TimWolla TimWolla merged commit 2d6a883 into php:master Aug 23, 2022
@TimWolla TimWolla deleted the random-test-cleanup branch August 23, 2022 22:05
@zeriyoshi
Copy link
Contributor

I have been busy for a while and could not respond. Sorry about that.

It may not be possible to react much until early September.

Thank you for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants