From 05b66fc35587f63889d1f11009f57632d05f678f Mon Sep 17 00:00:00 2001 From: Mickey Rose Date: Sat, 23 Jan 2016 20:21:05 +0100 Subject: [PATCH 1/3] refactor svg_parser_test - moved some boilerplate to helper struct test_parser - added REQUIRE(!parse...) to parsing error tests - changed parsing error tests to compare full error lists instead of just count and then individual messages (if count was different, you were left in the dark with no messages at all) - changed some double-quotes in errors to single-quotes (corresponding change to parser follows) --- test/data | 2 +- test/unit/svg/svg_parser_test.cpp | 246 +++++++++++++++--------------- 2 files changed, 120 insertions(+), 128 deletions(-) diff --git a/test/data b/test/data index 24844dead..e4178f3ce 160000 --- a/test/data +++ b/test/data @@ -1 +1 @@ -Subproject commit 24844dead6d23fd541aba147b69703a605fc7351 +Subproject commit e4178f3ce9798f93024a49d6cd0a87d98ec0f6b0 diff --git a/test/unit/svg/svg_parser_test.cpp b/test/unit/svg/svg_parser_test.cpp index e0bfd9ecc..3fee6a452 100644 --- a/test/unit/svg/svg_parser_test.cpp +++ b/test/unit/svg/svg_parser_test.cpp @@ -34,6 +34,42 @@ #include #include +namespace // internal +{ + struct test_parser + { + mapnik::svg_storage_type path; + mapnik::svg::vertex_stl_adapter stl_storage; + mapnik::svg::svg_path_adapter svg_path; + mapnik::svg::svg_converter_type svg; + mapnik::svg::svg_parser p; + + test_parser() + : stl_storage(path.source()) + , svg_path(stl_storage) + , svg(svg_path, path.attributes()) + , p(svg) + {} + + mapnik::svg::svg_parser* operator->() + { + return &p; + } + }; + + template + std::string join(C const& container) + { + std::string result; + for (auto const& str : container) + { + if (!result.empty()) result += "\n "; + result += str; + } + return result; + } +} + TEST_CASE("SVG parser") { SECTION("SVG i/o") @@ -49,142 +85,112 @@ TEST_CASE("SVG parser") { SECTION("SVG::parse i/o") { std::string svg_name("FAIL"); - - using namespace mapnik::svg; - mapnik::svg_storage_type path; - vertex_stl_adapter stl_storage(path.source()); - svg_path_adapter svg_path(stl_storage); - svg_converter_type svg(svg_path, path.attributes()); - svg_parser p(svg); - - if (!p.parse(svg_name)) + char const* expected_errors[] = { - auto const& errors = p.error_messages(); - REQUIRE(errors.size() == 1); - REQUIRE(errors[0] == "Unable to open 'FAIL'"); - } + "Unable to open 'FAIL'" + }; + + test_parser p; + REQUIRE(!p->parse(svg_name)); + REQUIRE(join(p->error_messages()) == join(expected_errors)); } SECTION("SVG::parse_from_string syntax error") { std::string svg_name("./test/data/svg/invalid.svg"); + char const* expected_errors[] = + { + "Unable to parse '\n\n'" + }; + std::ifstream in(svg_name.c_str()); std::string svg_str((std::istreambuf_iterator(in)), std::istreambuf_iterator()); - using namespace mapnik::svg; - mapnik::svg_storage_type path; - vertex_stl_adapter stl_storage(path.source()); - svg_path_adapter svg_path(stl_storage); - svg_converter_type svg(svg_path, path.attributes()); - svg_parser p(svg); - - if (!p.parse_from_string(svg_str)) - { - auto const& errors = p.error_messages(); - REQUIRE(errors.size() == 1); - REQUIRE(errors[0] == "Unable to parse '\n\n'"); - } + test_parser p; + REQUIRE(!p->parse_from_string(svg_str)); + REQUIRE(join(p->error_messages()) == join(expected_errors)); } SECTION("SVG::parse_from_string syntax error") { std::string svg_name("./test/data/svg/invalid.svg"); - - using namespace mapnik::svg; - mapnik::svg_storage_type path; - vertex_stl_adapter stl_storage(path.source()); - svg_path_adapter svg_path(stl_storage); - svg_converter_type svg(svg_path, path.attributes()); - svg_parser p(svg); - - if (!p.parse(svg_name)) + char const* expected_errors[] = { - auto const& errors = p.error_messages(); - REQUIRE(errors.size() == 1); - REQUIRE(errors[0] == "svg_parser::parse - Unable to parse './test/data/svg/invalid.svg'"); - } + "svg_parser::parse - Unable to parse './test/data/svg/invalid.svg'" + }; + + test_parser p; + REQUIRE(!p->parse(svg_name)); + REQUIRE(join(p->error_messages()) == join(expected_errors)); } SECTION("SVG parser color ") { std::string svg_name("./test/data/svg/color_fail.svg"); + char const* expected_errors[] = + { + "Failed to parse color: \"fail\"", + "Failed to parse SVG value: 'fail'", + "Failed to parse color: \"fail\"", + }; + std::ifstream in(svg_name.c_str()); std::string svg_str((std::istreambuf_iterator(in)), std::istreambuf_iterator()); - using namespace mapnik::svg; - mapnik::svg_storage_type path; - vertex_stl_adapter stl_storage(path.source()); - svg_path_adapter svg_path(stl_storage); - svg_converter_type svg(svg_path, path.attributes()); - svg_parser p(svg); - - if (!p.parse_from_string(svg_str)) - { - auto const& errors = p.error_messages(); - REQUIRE(errors.size() == 3); - REQUIRE(errors[0] == "Failed to parse color: \"fail\""); - REQUIRE(errors[1] == "Failed to parse SVG value: \"fail\""); - REQUIRE(errors[2] == "Failed to parse color: \"fail\""); - } + test_parser p; + REQUIRE(!p->parse_from_string(svg_str)); + REQUIRE(join(p->error_messages()) == join(expected_errors)); } SECTION("SVG - cope with erroneous geometries") { std::string svg_name("./test/data/svg/errors.svg"); + char const* expected_errors[] = + { + "parse_rect: Invalid width", + "Failed to parse SVG value: 'FAIL'", + "parse_rect: Invalid height", + "parse_rect: Invalid rx", + "parse_rect: Invalid ry", + "Failed to parse SVG value: '100invalidunit', trailing garbage: 'validunit'", + "unable to parse invalid svg ", + "unable to parse invalid svg with id 'fail-path'", + "unable to parse invalid svg with id 'fail-path'", + "parse_circle: Invalid radius", + "Failed to parse 'points'", + "Failed to parse 'points'", + "parse_ellipse: Invalid rx", + "parse_ellipse: Invalid ry", + }; + std::ifstream in(svg_name.c_str()); std::string svg_str((std::istreambuf_iterator(in)), std::istreambuf_iterator()); - using namespace mapnik::svg; - mapnik::svg_storage_type path; - vertex_stl_adapter stl_storage(path.source()); - svg_path_adapter svg_path(stl_storage); - svg_converter_type svg(svg_path, path.attributes()); - svg_parser p(svg); - if (!p.parse_from_string(svg_str)) - { - auto const& errors = p.error_messages(); - REQUIRE(errors.size() == 13); - REQUIRE(errors[0] == "parse_rect: Invalid width"); - REQUIRE(errors[1] == "Failed to parse SVG value: \"FAIL\""); - REQUIRE(errors[2] == "parse_rect: Invalid height"); - REQUIRE(errors[3] == "parse_rect: Invalid rx"); - REQUIRE(errors[4] == "parse_rect: Invalid ry"); - REQUIRE(errors[5] == "unable to parse invalid svg "); - REQUIRE(errors[6] == "unable to parse invalid svg with id 'fail-path'"); - REQUIRE(errors[7] == "unable to parse invalid svg with id 'fail-path'"); - REQUIRE(errors[8] == "parse_circle: Invalid radius"); - REQUIRE(errors[9] == "Failed to parse 'points'"); - REQUIRE(errors[10] == "Failed to parse 'points'"); - REQUIRE(errors[11] == "parse_ellipse: Invalid rx"); - REQUIRE(errors[12] == "parse_ellipse: Invalid ry"); - } + test_parser p; + REQUIRE(!p->parse_from_string(svg_str)); + REQUIRE(join(p->error_messages()) == join(expected_errors)); } SECTION("SVG parser double % ") { std::string svg_name("./test/data/svg/gradient-radial-error.svg"); + char const* expected_errors[] = + { + "Failed to parse SVG value: 'FAIL'" + }; + std::ifstream in(svg_name.c_str()); std::string svg_str((std::istreambuf_iterator(in)), std::istreambuf_iterator()); - using namespace mapnik::svg; - mapnik::svg_storage_type path; - vertex_stl_adapter stl_storage(path.source()); - svg_path_adapter svg_path(stl_storage); - svg_converter_type svg(svg_path, path.attributes()); - svg_parser p(svg); - - if (!p.parse_from_string(svg_str)) - { - auto const& errors = p.error_messages(); - REQUIRE(errors.size() == 1); - REQUIRE(errors[0] == "Failed to parse SVG value: \"FAIL\""); - } + test_parser p; + REQUIRE(!p->parse_from_string(svg_str)); + REQUIRE(join(p->error_messages()) == join(expected_errors)); } SECTION("SVG parser display=none") @@ -320,16 +326,10 @@ TEST_CASE("SVG parser") { std::ifstream in(svg_name.c_str()); std::string svg_str((std::istreambuf_iterator(in)), std::istreambuf_iterator()); - - using namespace mapnik::svg; - mapnik::svg_storage_type path; - vertex_stl_adapter stl_storage(path.source()); - svg_path_adapter svg_path(stl_storage); - svg_converter_type svg(svg_path, path.attributes()); - svg_parser p(svg); - p.parse_from_string(svg_str); - auto width = svg.width(); - auto height = svg.height(); + test_parser p; + REQUIRE(p->parse_from_string(svg_str)); + auto width = p.svg.width(); + auto height = p.svg.height(); REQUIRE(width == 100); REQUIRE(height == 100); } @@ -604,41 +604,33 @@ TEST_CASE("SVG parser") { SECTION("SVG missing def") { std::string svg_name("./test/data/svg/gradient-nodef.svg"); - using namespace mapnik::svg; - mapnik::svg_storage_type path; - vertex_stl_adapter stl_storage(path.source()); - svg_path_adapter svg_path(stl_storage); - svg_converter_type svg(svg_path, path.attributes()); - svg_parser p(svg); - REQUIRE(!p.parse(svg_name)); - auto const& errors = p.error_messages(); - REQUIRE(errors.size() == 2); - REQUIRE(errors[0] == "Failed to find gradient fill: MyGradient"); - REQUIRE(errors[1] == "Failed to find gradient stroke: MyGradient"); + char const* expected_errors[] = + { + "Failed to find gradient fill: MyGradient", + "Failed to find gradient stroke: MyGradient", + }; + + test_parser p; + REQUIRE(!p->parse(svg_name)); + REQUIRE(join(p->error_messages()) == join(expected_errors)); } SECTION("SVG missing id") { - std::string svg_name("./test/data/svg/gradient-no-id.svg"); + char const* expected_errors[] = + { + "Failed to find gradient fill: MyGradient", + "Failed to find gradient stroke: MyGradient", + }; + std::ifstream in(svg_name.c_str()); std::string svg_str((std::istreambuf_iterator(in)), std::istreambuf_iterator()); - using namespace mapnik::svg; - mapnik::svg_storage_type path; - vertex_stl_adapter stl_storage(path.source()); - svg_path_adapter svg_path(stl_storage); - svg_converter_type svg(svg_path, path.attributes()); - svg_parser p(svg); - - if (!p.parse_from_string(svg_str)) - { - auto const& errors = p.error_messages(); - REQUIRE(errors.size() == 2); - REQUIRE(errors[0] == "Failed to find gradient fill: MyGradient"); - REQUIRE(errors[1] == "Failed to find gradient stroke: MyGradient"); - } + test_parser p; + REQUIRE(!p->parse_from_string(svg_str)); + REQUIRE(join(p->error_messages()) == join(expected_errors)); } SECTION("SVG missing inheritance") From 8d7fca3236c555b6552e028edbaf13abb2f7b01d Mon Sep 17 00:00:00 2001 From: Mickey Rose Date: Sat, 23 Jan 2016 20:44:14 +0100 Subject: [PATCH 2/3] svg_parse_value - add 'px' unit, report trailing garbage - avoid operator comma in semantic action, refs #3249 --- src/svg/svg_parser.cpp | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/svg/svg_parser.cpp b/src/svg/svg_parser.cpp index e979e3f7b..6613d5cde 100644 --- a/src/svg/svg_parser.cpp +++ b/src/svg/svg_parser.cpp @@ -137,29 +137,36 @@ double parse_double(T & error_messages, const char* str) template double parse_svg_value(T & error_messages, const char* str, bool & percent) { - using namespace boost::spirit::qi; using skip_type = boost::spirit::ascii::space_type; using boost::phoenix::ref; - double_type double_; - char_type char_; - _1_type _1; + qi::double_type double_; + qi::lit_type lit; + qi::_1_type _1; double val = 0.0; - symbols units; + qi::symbols units; units.add + ("px", 1.0) ("pt", DPI/72.0) ("pc", DPI/6.0) ("mm", DPI/25.4) ("cm", DPI/2.54) ("in", (double)DPI) ; - if (!phrase_parse(str, str + std::strlen(str), - double_[ref(val) = _1, ref(percent) = false] + const char* cur = str; // phrase_parse modifies the first iterator + const char* end = str + std::strlen(str); + if (!qi::phrase_parse(cur, end, + double_[ref(val) = _1][ref(percent) = false] > - (units[ ref(val) *= _1] | - char_('%')[ref(val) *= 0.01, ref(percent) = true]), + lit('%')[ref(val) *= 0.01][ref(percent) = true]), skip_type())) { - error_messages.emplace_back("Failed to parse SVG value: \"" + std::string(str) + "\""); + error_messages.emplace_back("Failed to parse SVG value: '" + std::string(str) + "'"); + } + else if (cur != end) + { + error_messages.emplace_back("Failed to parse SVG value: '" + std::string(str) + + "', trailing garbage: '" + cur + "'"); } return val; } From e9fbe0724f172bb0ac5f794caca9b572dea8431a Mon Sep 17 00:00:00 2001 From: Mickey Rose Date: Sun, 24 Jan 2016 18:34:28 +0100 Subject: [PATCH 3/3] travis: fetch required git submodule pull requests --- .travis.yml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 0dcfac089..4be27fe6f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,7 +4,7 @@ sudo: false git: depth: 10 - submodules: true + submodules: false env: global: @@ -43,6 +43,15 @@ before_install: - export MASON_PUBLISH=${MASON_PUBLISH:-false} - if [[ ${TRAVIS_BRANCH} != 'master' ]]; then export MASON_PUBLISH=false; fi - if [[ ${TRAVIS_PULL_REQUEST} != 'false' ]]; then export MASON_PUBLISH=false; fi + - git submodule update --init --depth=10 || + git submodule foreach 'test "$sha1" = "`git rev-parse HEAD`" || + git ls-remote origin "refs/pull/*/head" | + while read hash ref; do + if test "$hash" = "$sha1"; then + git config --add remote.origin.fetch "+$ref:$ref"; + fi + done' + - git submodule update --init --depth=10 install: - if [[ $(uname -s) == 'Linux' ]]; then