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

Fix unit tests for Collector to work with Osmium #51

Closed
adamfranco opened this issue Jan 16, 2017 · 6 comments
Closed

Fix unit tests for Collector to work with Osmium #51

adamfranco opened this issue Jan 16, 2017 · 6 comments
Labels

Comments

@adamfranco
Copy link
Owner

9b0fa2a converted from using the imposm.parser to the Osmium PBF parser. In the process, the Collector API changed and dropped the parser_class argument that had been used for wiring in test structures for unit testing.

@adamfranco adamfranco added the bug label Jan 16, 2017
@darylmatuszak
Copy link
Contributor

I took a crack at this here

Will need a bit more work but would appreciate a peer review before I continue refining it.

The problems this cause seem to be limited to the tests in test_collector.py, preventing them from running due to the mentiond API change.

Since WayCollector can only parse via its parent's apply_file method, I took the mock data in the tests and used Osmium.SimpleWriter to generate files that can serve as fixtures.

Having done this, all tests except test_collector_doesnt_drop_unjoined_ways pass. Perhaps someone more familiar with the codebase can assist in determining if this failure is indeed genuine (I suspect it might be).

A few more notes:

  • This branch has a few other commits that are mainly custodial. Upon first running the tests, I noticed lots of warnings and mostly harmless errors regarding default arguments and calling fixtures. The only real errors I encountered were with the collector tests.

  • For any failing test, it will print out the full path to the file used. They are xml rather than pbf so to be human readable, and are all stored in a single directory common to the test run. So If you navigate to that directory, you can examine all the files used in detail.

  • The test will generate 36 warnings which seem be the result of using of using Osmium.SimpleHandler, can dig into this a bit more later but was focused on getting tests working first.
    DeprecationWarning: __int__ returned non-int (type bool). The ability to return an instance of a strict subclass of int is deprecated, and may be removed in a future version of Python. return f(*args, **kwds)

@adamfranco
Copy link
Owner Author

Thanks for your work on this @darylmatuszak ! :-) I've made a small tweak in 30de14e to get the tests running on Python 3.5 and will take a look at the failures and give it a more full review.

@adamfranco
Copy link
Owner Author

A note to myself: I needed to upgrade osmium from 2.14.3 to 2.15.4 in order to get the fix for wrong precision being written: osmcode/pyosmium#83

@adamfranco
Copy link
Owner Author

I took a look at test_collector_doesnt_drop_unjoined_ways and found that it was failing because the collections were getting returned in different orders than expected. The order of the collections in the result set isn't important, so I changed the test code in 540fc64 to filter out the individual collections based on their data rather than assuming their order. Now the test will pass if both exist and look correct independent of which order they appear in.

All tests are passing now in my test-compatability branch-- getting this working again is a great improvement. I'll be away from the computer for the weekend, but 👍 on continuing this work.

@darylmatuszak
Copy link
Contributor

Nice! Appreciate your expertise on sorting out that failure.

pyosmium is aware of the deprecation warning, and it seems to be coming from pybind11.

This is a cool project, and certainly one I will be getting a lot of mileage out of :)

As I continue to become more familiar, I hope to find further areas where I can make a contribution.

@adamfranco
Copy link
Owner Author

Thanks again for your help on this issue, @darylmatuszak! I've merged in your your work. I've been hard at work on a new in-browser map for Curvature over the past few months and haven't been giving the underlying code the love it deserves recently. Now that the new map is launched I'm hoping to find some more time for some features I've put on hold. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants