Add some FilesHelper tests #638
Conversation
|
Anything that helps confirm current behaviour will be a win, thanks! Not sure if the WP_Mock test classes you're using support the Definitely, the static methods make everything harder to test, so not opposed at all to converting those. Some have started to be used as ways of extending the plugin by users, so where applicable, we should add similar filters/hooks to keep that extensibility. |
|
Glad you brought up removing static methods. We should have one Singleton trait and convert all our static methods to standard ones IMO. The static methods aren't a big problem if they're entirely self contained, but when they call other static methods, testability is affected. |
|
No problem. Please don't hesitate to call me out on anything you see as a bad practice/poorly done. I somehow manage to write code from time to time, but still have very little knowledge of common design patterns, algorithms and such. As I'm seeing more contributors coming into the project, I'll be trying to make way for you all more to take over and I'll just use my domain knowledge / experience with the projects to help and work more on the support/docs/videos. |
|
It looks like we can set WP_Mock to use Patchwork, which can mock static methods. What sort of Singleton trait are we talking about? Searching, I find stuff like this and a class in Patchwork. I'm not sure what the advantage is over static methods? While we're on the subject of changing functions, what about the naming? Some are camel case and some are snake case. Shouldn't we be consistent one way or another? |
|
I'm unfamiliar with traits enough to comment on the Singleton trait, but would just like to ensure we aren't affecting performance by doing a lot of repeat class instantiation, so like I think we're doing with the Controller - checking for existence and only instantiating once (so like the 2nd example). The static methods are definitely a pain to test, where in previous versions of plugin, dependency injection helped with that and forced to split up and clean classes more. Would monkey-patching allow us to cover current code with tests and postpone any major refactoring of classes? Re function name casing - we should be able to add a rule PHPStan/phpcs to enforce camel case / camelback as per PSR-1: https://www.php-fig.org/psr/psr-1/ - suprised it's not in there already |
|
From the manual, "Declaring class properties or methods as static makes them accessible without needing an instantiation of the class." So there are zero instantiations with static methods. Made an issue for PSR checks at #641 |
yep, that was one of the reasons for using them, along with rapid initial development.
+1 |
|
All methods in FilesHelper should now be covered. I pulled in the popular testing package mikey179/vfsstream to help with filesystem tests. If you see any tests I'm missing let me know and I'll add them. |
|
Two changes i'd like to see applied to FilesHelper:
I'm also thinking we should modify our linting to not exclude the tests directory. Is there a reason we currently exclude it? |
|
Brilliant, thanks @Flynsarmy! Yeah, a lot of those bits of code haven't had a good review for a while, aside from adding typing. What is in there test-wise is left over from previous iterations of the plugin, IIRC, where things were quite different. That's more likely what morphed into https://github.com/WP2Static/static-html-output, but it's a bit hazy, to be honest. There was basically:
At least some of the URL detection logic will likely come out of this repo, ie. when trying to detect advanced URL patterns from WP, like custom post type pagination or similar. We'll be adding in/expanding logic similar to Static HTML Output to continually crawl and discover links from the "seeded" URLs this FilesHelper provides. There are 3 techniques that we need to combine to ensure coverage in both projects (well, all 3 projects if we include SimplerStatic, but that's least priority today):
cc @john-shaffer as I think he's got an idea for how to implement the 2nd option above in this repo in a bit different way than we have it in Static HTML Output, to better work with the caching mechanisms we have here already. |
| @@ -4,7 +4,7 @@ | |||
| "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", | |||
| "This file is @generated automatically" | |||
| ], | |||
| "content-hash": "7baeda382f69ca24a7267df1eaace8f1", | |||
| "content-hash": "ea1125c00ed02b6a9ede41b2b9f5adc8", | |||
There was a problem hiding this comment.
@john-shaffer @Flynsarmy suggestions to deal with composer.lock files going fwd in PR's? Seems cruel to not have any warning mechanism/.gitignore then reject from PRs...
There was a problem hiding this comment.
I've reverted composer.lock but I agree, there should be some automated restriction in place so changes are rejected through PRs. My bad.
There was a problem hiding this comment.
Ahh, and now we discover what happens when this is done. PR will automatically fail github workflow because composer.lock is bad. @leonstafford @john-shaffer any ideas how we should handle this? Should we just allow changes then after merge do a composer update and commit the updated one?
https://github.com/WP2Static/wp2static/pull/638/checks?check_run_id=1019840958
There was a problem hiding this comment.
@Flynsarmy I'm not sure. Suggest we find another project that's solved this issue already and borrow their technique 😸
There was a problem hiding this comment.
Thoughts: as WP2Static is not designed to run on production systems and honestly, we're not (I'm not) reviewing each package and each package's dependencies line for line as they update, it feels we're relying on other alerts as to when a 3rd party package would be compromised.
To buffer this, if we just include the lock files in PRs and merging to master (or move to a rolling dev branch as another improvement), then give a code lock period of a fixed time before tagging releases as a way to mitigate pushing out en masse any majorly compromised releases.
Alternatively, we move away from using any 3rd party packages (outside testing) in core and replicate the functionality they provide ourselves.
| @@ -71,6 +71,9 @@ public static function getListOfLocalFilesByDir( string $dir ) : array { | |||
| return $files; | |||
| } | |||
|
|
|||
| /** | |||
| * Ensure a given filepath has an allowed filename and extension. | |||
| */ | |||
| public static function filePathLooksCrawlable( string $file_name ) : bool { | |||
There was a problem hiding this comment.
@Flynsarmy open to improving fn naming here, for sure!
There was a problem hiding this comment.
Updated through 8aca024. Let me know if they're OK.
There was a problem hiding this comment.
minor edit I hate to ask for... sorry!
tests/unit/FileHelperTest.php
Outdated
|
|
||
| // Nested folder | ||
| $filepath = vfsStream::url('root/folder_2/folder_3'); | ||
| $expected = ['/folder_2/folder_3/file_5.jpg', '/folder_2/folder_3/file_6.jpg']; |
There was a problem hiding this comment.
Can we add some unicode filenames, long filenames, etc here to push boundaries?
There was a problem hiding this comment.
Added through 75ebb9a. While adding these I found some issues:
- getListOfLocalFilesByDir() doesn't return a complete list of files in a directory because it calls filePathLooksCrawlable() which has file extension and filename restrictions. Unsure if this is intentional or not. Recommend adding a warning in that methods docblock. I wrote a comment about this in one of the tests.
- getListOfLocalFilesByDir() does a urlencode() on filepath but not filename. I have a feeling this isn't the behavior that we want. Recommend fixing the function to urlencode() filename as well and adding information to the docblock describing that this will happen.
Let me know your thoughts.
There was a problem hiding this comment.
so more like getCrawlableFiles()?
rather than doc blocks, I'd prefer if we add a line in the code, assigning to aptly named variable, ie $encoded_filename=blah. The tests act as the real source of truth as to what's happening. The code will give a good idea, too. If the functions are too long that such assigned var gets lost from mental space, we can break them down further. I just don't want to rely on a docblock, which adds lines but does little to protect/enforce behaviour.
I'd start with a failing test for the encoded filename part and confirm what happens currently and confirm if that failure represents in a real world test of unicode/special chars filepath in WP
|
@Flynsarmy - all looks good to me! I added a few comments. I need to head to sleep and will await @john-shaffer and your advice re lock files. In case you want to work on renaming some of those functions for more clarity in the meantime, that's great. Else, will merge in tomorrow. |
|
@Flynsarmy for now, let's not make things difficult for you by worrying about |
|
It's probably easiest to just have a maintainer push a commit whenever there are any changes to the lock file. It should be pretty uncommon anyways, once we get past alpha. Name consistency is in #641, but it will take a while to get everything renamed. |
|
Great, thanks @john-shaffer! So, LGTM time here? @Flynsarmy did the |
Pushed through 025116d
I think we're good to go on this one. I think I'll let you handle renaming variables and tweaking things to perfection. I'd like to move onto adding tests in other areas. I'll use what you change as a template for the rest of my work. |
|
Thanks @Flynsarmy, @john-shaffer! Sorry this took a while, but hopefully, this will help speed up future PRs for others! @Flynsarmy I'm still a bit detached from code, so please don't wait for me. I'm a bit concerned about you writing tests and then that behaviour gets changed, but if you're OK to take the risk, I think it will help, regardless, as we'll better know how the system behaves today. Suggest doing class by class. And general rule of first to submit PR gets priority - so you should be safe :) I'll probably next work on some of the add-ons within this GH org which are out of date/not registering properly in core Add-ons menu. |
|
I'm fine with that. Changes are to be expected in an alpha. The tests are there to help us determine and assert current behavior. When we change the classes we can just update the tests to match the new functionality. |
I'm not super familiar with testing so this is my first attempt at using Mockery and WP_Mock. I added tests to a few FilesHelper methods. Not all the methods are tested yet but I figured I'd just check you guys are happy with how I've done things here before continuing.
I'm not sure yet how to test
getListOfLocalFilesByDiras it relies on the result offilePathLooksCrawlablewhich is another static method - so I can't mockfilePathLooksCrawlable's response.I also don't like the huge arrays needed in WP_Mock::onFilter. I created an issue in their repo here describing my problem with it and a potential fix. Perhaps a solution to my problem already exists and I'm just not aware of it.
And finally I'm not sure if the output from
testCleanDetectedURLsis intended to work the way it does, I just added tests for everything I could think of so we can see as plainly as possible how it's currently working. Up to you guys to decide if you like the way starting/trailing slashes work with it.