search

CartoDB / Windshaft

A Node.js map tile library for PostGIS and torque.js, with CartoCSS styling
BSD 3-Clause "New" or "Revised" License
310 stars 81 forks source link

acceptance/server.js test failing #21

Closed strk closed 12 years ago

strk commented 12 years ago

uncaught undefined: Error: Images not equal(18.0864): /tmp/6223843058105558 /home/src/cartodb/Windshaft/test/fixtures/test_table_13_4011_3088.png at /home/src/cartodb/Windshaft/test/support/assert.js:29:27 at ChildProcess.exithandler (child_process.js:100:7) at ChildProcess.emit (events.js:67:17) at ChildProcess.onexit (child_process.js:192:12)

See the image difference here: http://strk.keybit.net/tmp/acceptance_server_imgdiff.png

Looks like bigger point size is expected.

strk commented 12 years ago

for the record: this test hangs expresso unless I force cache clearance (timeout removal)

strk commented 12 years ago

alright after fixing all the hanging and having ported tests to mocha I could put here a full report of failing tests. 3 tests find expected/obtained image differences (like the one in original report) and another 2 find the UTF8 grid being different from expected.

I'd love to hear if it works for others.

Command to run, from top-level dir:

echo "port 6333" | redis-server -  > /tmp/redis_test.log &
mocha -u tdd --ignore-leaks test/acceptance/server.js
strk commented 12 years ago

I confirm all 3 image mismatches seem related to applied style. We expect bigger points and obtain smaller ones. I've no easy way to check the UTF grid but could very likely be caused by the same unexpected style being used.

strk commented 12 years ago

Omitting the --ignore-leaks switch also detects leaks in the "server post'ing bad style returns 400 with error" test.

strk commented 12 years ago

where is the default style set ? could this have to do with external code ?

strk commented 12 years ago

running a single failing test (the first) leaves this key in redis:

redis-cli -p 6333 get "map_style|windshaft_test|test_table" "{\"style\":\"#test_table {marker-fill: #FF6600;marker-opacity: 1;marker-width: 8;marker-line-color: white;marker-line-width: 3;marker-line-opacity: 0.9;marker-placement: point;marker-type: ellipse;marker-allow-overlap: true;}\",\"xml\":\"<?xml version=\"1.0\" encoding=\"utf-8\"?>\n<!DOCTYPE Map[]>\n<Map srs=\\"+proj=merc +lon_0=0 +lat_ts=0 +x_0=0 +y_0=0 +a=6378137 +b=6378137 +nadgrids=@null +units=m +no_defs\\">\n\n\n<Style name=\\"test_table\\" filter-mode=\\"first\\">\n \n <MarkersSymbolizer fill=\\"#ff6600\\" opacity=\\"1\\" width=\\"8\\" stroke=\\"#ffffff\\" stroke-width=\\"3\\" stroke-opacity=\\"0.9\\" placement=\\"point\\" marker-type=\\"ellipse\\" allow-overlap=\\"true\\" />\n \n\n<Layer\n name=\\"test_table\\"\n srs=\\"+proj=merc +lon_0=0 +lat_ts=0 +x_0=0 +y_0=0 +a=6378137 +b=6378137 +nadgrids=@null +units=m +no_defs\\">\n test_table\n \n <Parameter name=\\"user\\"><![CDATA[postgres]]>\n <Parameter name=\\"host\\"><![CDATA[127.0.0.1]]>\n <Parameter name=\\"port\\"><![CDATA[5432]]>\n <Parameter name=\\"type\\"><![CDATA[postgis]]>\n <Parameter name=\\"geometry_field\\"><![CDATA[the_geom_webmercator]]>\n <Parameter name=\\"extent\\"><![CDATA[-20005048.4188,-9039211.13765,19907487.2779,17096598.5401]]>\n <Parameter name=\\"srid\\"><![CDATA[3857]]>\n <Parameter name=\\"max_size\\"><![CDATA[10]]>\n <Parameter name=\\"table\\"><![CDATA[test_table]]>\n <Parameter name=\\"dbname\\"><![CDATA[windshaft_test]]>\n \n \n\n\"}"

I dunno where width="8" comes from

strk commented 12 years ago

I measured the diameter of the expected marker and is 16. Also I see that marker-size="8" is the (expected) default. Can it be we're expecting the wrong answer ? Or some dependent module changed interpretation of the marker size from radius to diameter ?

springmeyer commented 12 years ago

Yes, see https://github.com/mapnik/mapnik/issues/1134

strk commented 12 years ago

alright then I guess we could expect the smaller markers, since I see the thing was backported to 2.0.x. Is there an officially released mapnik version with the bugfix in it ?

Surely does sound like a big change to put in a micro release as it would break the style of any application using markers...

strk commented 12 years ago

For the record: the failures I'm having with 2.0.2-pre -- not sure how to handle this. I think it is correct for the test to fail (semantic change). The README for Windshaft says to install "mapnik-2.0" so the change in 2.0.2 would hurt.

strk commented 12 years ago

OTOH, I'm supposed to look at making Windshaft work with Mapnik-2.1 (#14) so the problem will come back.

springmeyer commented 12 years ago

I don't think testing marker interactivity is super meaningful here. Windshaft uses node-mapnik, which itself tests grid rendering and interactivity. I'd say simply test that grids can be rendered without throwing an exception and call it good.

strk commented 12 years ago

Both rendering and interactivity (the grid, you mean?) is tested by the acceptance test. It is also tested that the default style is what we expect, and that the resulting tile looks like we expect.

I'm glad we found this out. If we drop the tests we'll not find out next time a semantic change in a dependency happens.

strk commented 12 years ago

For the record, according to https://github.com/mapnik/mapnik/blob/master/CHANGELOG.md the Marker interpretation change happened in 2.0.1

tokumine commented 12 years ago

ahah - well tracked down. I only ran the tests against my mapnik 2.0 install.

strk commented 12 years ago

so what do we do ? we stick to 2.0.0 or upgrade and warn users ?

tokumine commented 12 years ago

I +1 stay on 2.0 until we can port users styles for them

On Mon, Jun 18, 2012 at 3:24 PM, strk < reply@reply.github.com

wrote:

so what do we do ? we stick to 2.0.0 or upgrade and warn users ?

Reply to this email directly or view it on GitHub: https://github.com/Vizzuality/Windshaft/issues/21#issuecomment-6397124

strk commented 12 years ago

mind you: it is 2.0.0, not "2.0" (unless it gets reverted in the 2.0 branch).

What you mean by porting users styles ? We don't have visibility of all styles being used trough the mapping API, do we ? It looks to me that the only way to handle such a change without breaking any existing use would be to maintain an old and new API version, encoding it in the URL entry point.

tokumine commented 12 years ago

yes - I meant 2.0.0

I guess it depends on what level of support we want to still leave for 2.0.0 mapnik in windshaft. What do you think?

S

On Mon, Jun 18, 2012 at 3:32 PM, strk < reply@reply.github.com

wrote:

mind you: it is 2.0.0, not "2.0" (unless it gets reverted in the 2.0 branch).

What you mean by porting users styles ? We don't have visibility of all styles being used trough the mapping API, do we ? It looks to me that the only way to handle such a change without breaking any existing use would be to maintain an old and new API version, encoding it in the URL entry point.

Reply to this email directly or view it on GitHub: https://github.com/Vizzuality/Windshaft/issues/21#issuecomment-6397342

strk commented 12 years ago

On Mon, Jun 18, 2012 at 07:39:42AM -0700, tokumine wrote:

yes - I meant 2.0.0

I guess it depends on what level of support we want to still leave for 2.0.0 mapnik in windshaft. What do you think?

I guess my concern is more to do with deploy than code.

On the code front I think it is fine to drop support for 2.0 in master as soon as 2.1 is out. But such switch should probably go with an appropriate version increment (major? minor? something different from a bugfix release versioning) and branching to account for whoever will be willing to still work with older version of mapnik.

--strk;

Sent from our free software http://www.gnu.org/philosophy/free-sw.html

tokumine commented 12 years ago

Yes, this sounds good - at least we can develop in isolation here and minimise impacts using versions. It's quite a big change, and will probably co-incide with support for later node versions so either major/minor would work I think.

On Mon, Jun 18, 2012 at 3:48 PM, strk < reply@reply.github.com

wrote:

On Mon, Jun 18, 2012 at 07:39:42AM -0700, tokumine wrote:

yes - I meant 2.0.0

I guess it depends on what level of support we want to still leave for 2.0.0 mapnik in windshaft. What do you think?

I guess my concern is more to do with deploy than code.

On the code front I think it is fine to drop support for 2.0 in master as soon as 2.1 is out. But such switch should probably go with an appropriate version increment (major? minor? something different from a bugfix release versioning) and branching to account for whoever will be willing to still work with older version of mapnik.

--strk;

Sent from our free software http://www.gnu.org/philosophy/free-sw.html

Reply to this email directly or view it on GitHub: https://github.com/Vizzuality/Windshaft/issues/21#issuecomment-6397772

strk commented 12 years ago

On Mon, Jun 18, 2012 at 07:51:14AM -0700, tokumine wrote:

Yes, this sounds good - at least we can develop in isolation here and minimise impacts using versions. It's quite a big change, and will probably co-incide with support for later node versions so either major/minor would work I think.

Ok so for the sake of this specific ticket the task becomes encoding the strong requirement for mapnik 2.0.0.

--strk;

Sent from our free software http://www.gnu.org/philosophy/free-sw.html

springmeyer commented 12 years ago

sticking to 2.0.0 sounds completely reasonable. Or, depending on what we work out at https://github.com/mapnik/mapnik/issues/1134, requiring a hard dependency on 2.0.2, which can be released promptly (this week). Only gocha with sticking with 2.0.0 is this bug: https://github.com/mapnik/mapnik/issues/903, and the lacking postgis 2.0 support, both things fixed in >= 2.0.1.

strk commented 12 years ago

I confirm acceptance test passing after installing 2.0.0 (as of 070fe308)

strk commented 12 years ago

Interesting enough, when running the test from within run_test.sh there are failures. Maybe the server isn't cleaned down from previous tests (could have to do with the reported leaks).

strk commented 12 years ago

FYI: the first of 8 failures is:

 1) server get'ing blank style returns default style:
    AssertionError: undefined. Invalid response body.
   Expected: {"style":"#test_table {marker-fill: #FF6600;marker-opacity: 1;marker-width: 8;marker-line-color: white;marker-line-width: 3;marker-line-opacity: 0.9;marker-placement: point;marker-type: ellipse;marker-allow-overlap: true;}"}
   Got: {"style":"#test_table {polygon-fill:#FF6600; polygon-opacity: 0.7; line-opacity:1; line-color: #FFFFFF;}"}
strk commented 12 years ago

On a closer look the offending interaction is between unit/render_cache.test.js and acceptance/server.js -- running the render_cache before the server test triggers a failure in the latter, inverting the order is fine.

strk commented 12 years ago

269e682 closes this. Leaks will be handled in #23, ordering in #24