search

catchpoint / WebPageTest.api-nodejs

WebPageTest API wrapper for NodeJS
MIT License
1.64k stars 173 forks source link

"--noads" option does not appear to work #47

Open okor opened 9 years ago

okor commented 9 years ago

Thanks for your work on this is been very helpful. I have some questions / possible bug reports.

Here you can see I've submitted two tests with the cli. One with the --noads option and one without. But both results clearly render an Apple ad. In comparison, I'm running Chrome with the AdBlock Plus plugin and the ad is being blocked as expected. So if the option is suppose to be using the AdBlock Plus block list, I think this should work.

noads-

1) Am I using the option correctly? 2) The README states Test (works for test command only) for all of the options in which the --noads option is listed. I assume this means it should also work for the node library .runtTest(<someurl>, { noads: true|false }, ...) equivalent as well ? 3) I don't see this option in the webpagetest.org UI but I assume that maybe you are injecting some variation of the adblock plus block list into the Block Requests Containing... option?

This library has some pretty excellent CI and appears to be passing all tests. If this is a bug, perhaps in addition to fixing it we could add some coverage for this? ci

Let me know if I can lend a hand.

pmeenan commented 9 years ago

It's an issue with WebPageTest itself and not the API wrapper. The ads blocking list hadn't been maintained/updated for a couple of years so it was removed. It might be worth updating but explicitly selecting the content you'd like to block tends to work better.

okor commented 9 years ago

Ok, I see.

This seems like a really powerful feature if maintained. In my case, because I work on sites that have ads which may load something different each time the page is loaded ... each ad having different performance characteristics ... it's very valuable to be able to have an ad-less baseline of production performance vs with ads. That said, sending a block list manually seems fine too. Given the following block list https://easylist-downloads.adblockplus.org/easylist.txt how should I format that so that WPT understands it? Also, if you have a better idea than simply grabbing this list (and caching to disk/memory for say 24 hours) and cleaning up the format, I'd love any advice.

okor commented 9 years ago

I think it makes sense to remove this option from the list: https://github.com/marcelduran/webpagetest-api/pull/48

pmeenan commented 9 years ago

Do your sites use their own ad-loader that then goes out and decides what ad to load or is it a back-end decision and injected directly into the pages? Usually you just have to block the few root ad calls which then block everything downstream.

The WPT blocking doesn't support everything that the adblock list supports (regex, DOM nodes, CSS selectors, etc). It only supports substring matches on the URLs. If you pull out just the substring URLs then you can use that as a block list (space separated) though there's a chance that it will add measurable overhead to scan that long of a list for every network request.

okor commented 9 years ago

We do in fact use a single loader. The url is always the same to a single item block list should work perfectly. Thanks for the idea!

okor commented 9 years ago

The block list method works well in my case. So I'm happy.

I would however really like to encourage you (or the maintainer) to accept my pull request to remove the advertised "--no-ads" option from the README. That should help to prevent similar confusion.

pmeenan commented 9 years ago

Yep, sorry, I don't have permissions here - I just run the WebPageTest side of things (and it was removed from the API docs so I think we're good over there).

Marcel, if you get a chance can you look at the PR to remove the noads support?