search

mapseed / api

Legacy api for the Mapseed platform
https://mapseed.org
GNU General Public License v3.0
7 stars 7 forks source link

Fix tests on DRF3/Django 1.8 #130

Closed zmbc closed 6 years ago

zmbc commented 6 years ago

This PR fixes the automated tests, but I am sure there are at least 3-4 more problems (if not more). What's our testing strategy? How can we run this against real maps?

goldpbear commented 6 years ago

RE: testing this on real maps-- I'd say we should deploy this branch on the dev api and set up one or more of the dev mapseed urls (dev.mapseed.org, dev2.mapseed.org, etc.) with different flavors deployed against the dev api.

I can get all that set up later toady.

modulitos commented 6 years ago

FYI I am still reviewing this PR - my Django/DRF knowledge is a bit rusty :p Although I should finish with reviewing/testing this PR today.

@zmbc

I am sure there are at least 3-4 more problems (if not more).

Can you elaborate a bit more on these problems? I will see if I can surface any problems while testing out this PR.

What's our testing strategy? How can we run this against real maps?

I plan to do some user acceptance testing locally, and if all goes well, we can merge in this PR and deploy our django1.8-drf3-upgrades feature branch to dev-api as mentioned by @goldpbear for further user acceptance testing.

If possible, maybe we can add more unit tests if there are any paths that we missed.

@goldpbear

I'd say we should deploy this branch on the dev api and set up one or more of the dev mapseed urls

I agree, although I am not sure if that is required before merging in this PR to the django1.8-drf3-upgrades feature branch.

Setting up the dev api on a staging site would be a great idea before we merge our feature branch into master. But before we do that, I think we still have a couple tasks to complete:

I am happy to help out with these tasks once this PR is merged.

goldpbear commented 6 years ago

@modulitos

I agree, although I am not sure if that is required before merging in this PR to the django1.8-drf3-upgrades feature branch.

ok, that makes sense-- we can test the whole thing before merging to master.

zmbc commented 6 years ago

Can you elaborate a bit more on these problems?

If I knew what they were I would fix them :laughing:

I was just saying, given the amount of code change here, that there are likely to be several more issues that aren't being caught by our tests.

zmbc commented 6 years ago

Alright, I think I've dealt with everything pointed out @modulitos.

Additionally, do you know what's going on with the Circle CI? It says it's failing in the pip install step.

goldpbear commented 6 years ago

This is great! @modulitos -- re: the 401 errors on comments and supports: did you confirm that the same operations with the same permissions work on master? I ask because permissions on comments and supports seem to be a little janky to begin with and this PR may not have introduced the problem. See this issue: https://github.com/mapseed/api/issues/127

Not sure if that applies here, but I thought it was worth mentioning.

zmbc commented 6 years ago

@modulitos I'd be in favor of fixing any bugs before merging.

modulitos commented 6 years ago

The dataset list failure is just a bug; I guess that view doesn't have tests. Should I push code and have you re-test? (I'll also look for similar bugs in the views.)

Yes, feel free to push up a fix and I"ll be happy to retest. Even better, maybe you can add a test for that view :pray: But don't worry about it if you think it is out of scope for this PR.

I'll wait on the permissions issue pending @goldpbear's comment; sounds like there is some stuff here I don't understand. If it's confirmed to only happen on this branch I can look into it.

The 401 errors on comments and supports were not related to this PR - thanks @goldpbear for the catch! I was able to get the comments and supports to work by adding an extra Origin permission, so I think this is a separate issue related to #127. We should be able to address these permissions issues after the upgrade.