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

Add fileExtensionsToIgnore and filenamesToIgnore advanced options #833

Merged

Conversation

john-shaffer
Copy link
Contributor

Move these features from the advanced crawling addon into core.

@john-shaffer john-shaffer force-pushed the feature-ignore-files-options branch 2 times, most recently from 78c1803 to 88aeb95 Compare October 27, 2021 00:54
@john-shaffer
Copy link
Contributor Author

I'm pretty sure this is just a problem with the mocking, and the actual code is fine. I don't know how to fix the mocks, though.

'.yarn',
'.zip',
];
$file_extensions_to_ignore = CoreOptions::getLineDelimitedBlobValue(
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do away with the temporary variable here?

@leonstafford
Copy link
Contributor

Checking the mocks issue. Added cpl comments.

Are we logging status of each path during crawl already (may have been in Static HTML Output). Wonder if logging when it is ignored due to user/plugin rule can be added if so. Else, maybe dump all the ignore rules for a job into regular logs somewhere?

@leonstafford
Copy link
Contributor

For our test with the error, testGetListOfLocalFilesByDir, we're getting DB related issue. For this unit/integration test, around local file system stuff, I don't think we should be hitting $wpdb in CoreOptions, so we should be able to mock CoreOptions::getLineDelimitedBlobValue() with our intended string and have it return what we want for our test.

Lemme see if we do similar elsewhere to borrow from

@leonstafford
Copy link
Contributor

We have the one in SimpleRewriterTest, could you try something like that?

@john-shaffer
Copy link
Contributor Author

john-shaffer commented Oct 27, 2021

I really, really don't like the mocking because of how completely unrelated things blow up when you change an implementation detail. I've been having success with writing two levels of tests: Functional unit tests on one level, which are inherently easy to test, and a second level that depends only on the external API. The external API should usually be backwards-compatible anyways, so when tests blow up it reveals an actual flaw somewhere. The "false positive rate" (when tests blow up but there is nothing wrong with the code) is close to zero. In other words, I only allow the tests to depend on things that I explicitly don't want to change, so I'm free to add features and change implementation details without patching up old tests. Also, at this level I have always been able to test against the actual backing services rather than mocks.

In this case, I think we should make a functional version of filePathLooksCrawlable that takes $filenames_to_ignore and $file_extensions_to_ignore as arguments. This will be very easy to unit test, and it should be much more efficient since we only have to compute those once for the entire crawl instead of for each individual file.

@john-shaffer
Copy link
Contributor Author

Are we logging status of each path during crawl already (may have been in Static HTML Output). Wonder if logging when it is ignored due to user/plugin rule can be added if so. Else, maybe dump all the ignore rules for a job into regular logs somewhere?

Advanced crawling addon kept track of status, but I haven't gotten to bringing that into core yet.

@john-shaffer john-shaffer force-pushed the feature-ignore-files-options branch from 88aeb95 to 3e637a7 Compare November 6, 2021 18:50
@john-shaffer
Copy link
Contributor Author

I rewrote things to be more easily testable. This does remove the tests for filters, but I think that this is the wrong place for that. We should have options filters handled by CoreOptions for all options, not scattered around.

@john-shaffer
Copy link
Contributor Author

I added a filter test in #835

@leonstafford leonstafford merged commit 4473d6e into elementor:develop Nov 8, 2021
@john-shaffer john-shaffer deleted the feature-ignore-files-options branch November 9, 2021 02:51
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