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

Refactor test helpers and test files #104

Merged
merged 6 commits into from
Feb 26, 2024

Conversation

haszi
Copy link
Contributor

@haszi haszi commented Feb 25, 2024

Refactor TestRender so its dependencies can be passed to its constructor.
Restructure test directories to follow the structure of Phd classes.
Rename test helper classes to indicate which package and format they belong to.
Move TestRender and test helper classes to phpdotnet/phd directory so that the autoloader can load them.
Refactor setup.php (basic test setup code) and all tests to work with the new files, directory structure and to only include necessary code.

While there is room for improvement (such as sub-namespace and corresponding directory for test helpers) these changes are enough to easily test any format or the indexer with a handful of lines of setup code.

haszi added 6 commits February 25, 2024 20:40
Add Format as an optional constructor dependency to TestRender.
Check format's methods conditionally.
Refactor tests to inject formats.
Add Config as a constructor dependency to TestRender.
Refactor tests to inject Config.
Refactor TestRender to use only constructor injected objects.
Rename test format helper classes to indicate which package they belong to.
Move TestRender and all format helper classes into the phpdotnet phd directory to enable autoloading.
Remove all unnecessary lines from setup.php.
Restructure test directory to follow the structure of the tested classes.
@haszi haszi force-pushed the Refactor-test-helpers-and-test-files branch from 0020cc9 to c5fce08 Compare February 25, 2024 19:59
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM thank you :)

"xml_file" => $xml_file,
"output_dir" => __DIR__ . "/output/",
);
Config::init(["xml_file" => $xml_file]);
Copy link
Member

Choose a reason for hiding this comment

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

One improvement for later would maybe to make Config a Value Object and use named params to set the config keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean something like a readonly Value Object with named constructor parameters? Because that sounds like something that might make juggling these configurations easier. Unfortunately the properties of Config are not setup in the beginning of rendering so that it can easily be changed to a readonly VO but with some refactoring that could be done too.

I'll make a note of this and will probably start refactoring parts of this one by one (such as Options_Parser to return an array instead of setting Config directly, Index's database to be constructor injected so it can be added to Config outside of Index as well, moving hardcoded css urls out of PHP's XHTML format so it also can be added to Config at the very beginning of the script, output_formats also set at the beginning of the script, etc.).

Copy link
Member

Choose a reason for hiding this comment

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

Basically yes :) having a magic dictionary array with keys is hard to reason about, could be fixed by using a static analyser array shape, but I doubt any static analysis tool is going to enjoy PhD in the state it currently is :')

@Girgias Girgias merged commit b703f50 into php:master Feb 26, 2024
8 checks passed
@haszi haszi deleted the Refactor-test-helpers-and-test-files branch February 26, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants