You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pagespeed.apache.org by GitBox <gi...@apache.org> on 2020/04/10 17:24:05 UTC
[GitHub] [incubator-pagespeed-mod] hallerm opened a new pull request #1987:
Improve beacon data decoding
hallerm opened a new pull request #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987
Critical image beacon data that does not conform to the expected format
should be ignored. Update the JSON handling so that fields that do not
have the expected type cause the critical image data to be ignored.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] hallerm commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
hallerm commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612198233
Thank you both. I expect this to be very helpful.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] hallerm commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
hallerm commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612212820
Ok, I added a test. And I updated to put all of the JsonCpp code inside the try. I'm not sure how to induce errors in those other spots without a bit of research, but at least it covers the one that we were getting hit with.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] hallerm commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
hallerm commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612159586
The updated version only changes which classes exception of exceptions are being thrown. The default is to `#define JSON_USE_EXCEPTION 1`. In the current version it will then throw `std::runtime_error`. In the new version this is changed to `Json::Exception` extends `std::exception`. This is why the catch is for `std::exception&`: so that if JsonCpp is upgraded it does not quietly break.
Regarding tests, I am having difficulty figuring out how to run the tests. The instructions at https://github.com/apache/incubator-pagespeed-mod/wiki/Development are failing a clone of git://git.apache.org/apr.git, apr-util, httpd. I've been searching the makefiles but so far haven't stumbled upon a target to run the tests. Is there a simple target to run the unit tests?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] oschaaf commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
oschaaf commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612197355
If you want to add a c++ unit tests for this, you should be able to just run `make pagespeed_automatic_test mod_pagespeed_test`, and then run either of those directly, sure.
You may be able to use `--gtest_filter=Foo.Bar` as an argument to those, to narrow done to just the one(s) you added for convenience.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] oschaaf commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
oschaaf commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612172890
re: tests
A system test would probably be most convenient, possibly it is prudent to add one in
https://github.com/apache/incubator-pagespeed-mod/blob/master/pagespeed/system/system_tests/critical_image_beaconing.sh
You can run all system tests via `make apache_test` in `devel/`.
Or you can run the release scripts via `install/build_release.sh --verbose --skip_psol `. This will run the full test set, as well as sort out installing the deps (if you don't want all these deps installed, an (ubuntu 16) vm is convenient for that).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] hallerm commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
hallerm commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612194981
Nevermnd. I found that I can just ignore the install error and run `mod_pagespeed/out/Debug/mod_pagespeed_test` directy.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] oschaaf merged pull request #1987:
Improve beacon data decoding
Posted by GitBox <gi...@apache.org>.
oschaaf merged pull request #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] oschaaf commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
oschaaf commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612174396
Also, thanks for looking into the `JSON_USE_EXCEPTION` more closely. I didn't expect it to work like that :-) Should we then also wrap the `parse()` calls?
I went out and updated the jsoncpp dep to the latest, this seems to just work. I'll can look into that separately, as it's fair to say that is of scope here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] oschaaf commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
oschaaf commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612199979
FWIW, off topic, but I also pushed out a simple dev docker image in https://github.com/apache/incubator-pagespeed-mod/commit/dfa4344d5c3d8c0490ec24c1a88d05fdc8bd36f6
(see the commit description on how I used it)
Maybe we should attempt to land such a thing at some point
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] hallerm commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
hallerm commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612191928
The `make apache_test` fails as it tries to become root to install memcached. Is there really no way to run simple unit tests?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] oschaaf commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
oschaaf commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612363959
Thanks @hallerm !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] oschaaf commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
oschaaf commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612139725
Thanks! It would be great to cover this with a test to make sure this stays put, would you be up for adding one?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] hallerm commented on issue #1987: Improve
beacon data decoding
Posted by GitBox <gi...@apache.org>.
hallerm commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612180054
> Also, thanks for looking into the `JSON_USE_EXCEPTION` more closely. I didn't expect it to work like that :-) Should we then also wrap the `parse()` calls?
Agreed, it should be assumed that it throws exceptions from all of its functions.
I'll push another commit with that and hopefully a test soon.
> I went out and updated the jsoncpp dep to the latest, this seems to just work. I'll can look into that separately, as it's fair to say that is of scope here.
Good, we are thinking alike on this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] jmarantz commented on issue #1987:
Improve beacon data decoding
Posted by GitBox <gi...@apache.org>.
jmarantz commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612197929
I think the test you want for this is probably in pagespeed_automatic_test rather than mod_pagespeed_test. The latter has system-y things like memcached, the former has algorithmic thinks like critical_image_finder. 'make apache_test' runs both, but it makes sense for you to just focus on the new test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-pagespeed-mod] jmarantz commented on issue #1987:
Improve beacon data decoding
Posted by GitBox <gi...@apache.org>.
jmarantz commented on issue #1987: Improve beacon data decoding
URL: https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612424969
+1 thanks!
On Sat, Apr 11, 2020 at 4:15 AM Otto van der Schaaf <
notifications@github.com> wrote:
> Thanks @hallerm <https://github.com/hallerm> !
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/incubator-pagespeed-mod/pull/1987#issuecomment-612363959>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAO2IPPOZH5CATA4VM5TEYDRMARKXANCNFSM4MFSTGNA>
> .
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services