Tests Tests Tests -- Union is broken

[ranchodeluxe]

It's great that all this new development is happening, but why don't I see tests around the stuff we are merging into master?

Currenlty the union is broken

[mapsam]

Because none of us know how to write them 😞 But I also think fixing union requires us to work on #5 before we can actually fix it.

[alukach]

Yup. it does seem broken. Weird because I tested it before pushing... Definitely highlights the need for more testing.

@thebigspoon Can you describe how it's broken? I get this error:

Uncaught TypeError: Cannot read property 'type' of undefined  turf.min.js:10 

[mapsam]

That's breaking because turf is looking for a feature not a featureCollection. I think the big issue here is realizing that we need to actually write tests when we add new features. It's not going to happen automatically.

[alukach]

Yes, accompanying tests should be a requirement for all pull requests.

[alukach]

@thebigspoon Alright, trying again, I'm unable to reproduce the error. Can you post the files that you're trying? I think @svmatthews may be correct re: this being about feature vs featureCollection.

Here are two files that I was able to successfully union: https://gist.github.com/alukach/0bf4471e44a0243b66db

[mapsam]

Sure thing. I'm trying to union two newly buffered geometries, which is where the problem exists. You can get the two original (eastbay & sf) files in this gist - below is a screencap of the process that results in the error:

[alukach]

So, were these always "broken"? No lost functionality, right?

I'm going to start taking a stab at #22 and #23 soon. I'll add those files in the "known failures" pile. I'm okay with there being failures at this current state, but we should make sure that we can pin down what fails (and hopefully why), and what doesn't, to make sure that there are at least no regressions.

[mapsam]

Union used to work when before the new menu system was put in place ... but it was a super gross hack looking for the feature[0] if it was a featureCollection. Nothing that should have made its way into production. I'm also fine with this being broken now - we'll have to discuss how to properly deal with all of this especially since Turf.js doesn't take featureCollections with some of its modules.

[alukach]

Ah, okay, right, I do remember seeing these two lines and not knowing what they did. It's something that should have been brought up in the PR, my mistake. Well, we can add a kludge to make it work for now (driven by options), or move on to consider better solutions. I had another issue with buffer (Turfjs/turf-buffer#22) that is resolved when turf-buffer is upgraded (which has still yet to happen). Maybe this problem will just go away on upgrade as well? Worth testing before committing dev time on our end.

[mapsam]

Heh, I submitted an issue to turf-union as well Turfjs/turf-union#8. It looks like some of the current turf modules are relying on jsts and don't account for all of the use cases. Hopefully it is upgraded soon ... or we could help out!