koopjs / koop-provider-agol

ArcGIS Online provider for Koop (public services only).
Other
12 stars 10 forks source link

Refactoring the model and controller and worker #46

Closed chelm closed 9 years ago

chelm commented 9 years ago

Okay this is pretty big. It's not done yet but tests pass. Problems is that test are not awesome. I'm to spend another day working on getting meaningful tests before we want to merge.

ungoldman commented 9 years ago

Looks like c73e344e940a525452e3cc1012814bd7a8361c4e got pushed to master

chelm commented 9 years ago

@ngoldman Yeah I pushed that to master to get the tests passing. Actually not sure how the tests passed before. The logger issue may have been created in the koop/lib refactor.

I rebase master onto this branch and then sync'd the PR

chelm commented 9 years ago

@dmfenton my local tests are really rocking steady. I'm pretty happy with the stability. YOu should merge when you are ready and we should cut a 1.1.0 release.

dmfenton commented 9 years ago

locally I see these tests failing:

  1) AGOL Controller getting large feature data w/a format "before all" hook:
     TypeError: Attempted to wrap undefined property exportFile as function
      at Context.<anonymous> (test/controller-test.js:378:13)

  2) AGOL Controller getting large feature data w/a format "after all" hook:
     TypeError: undefined is not a function
      at Context.<anonymous> (test/controller-test.js:397:24)

  3) AGOL Model when building a geohash "before all" hook:
     TypeError: Attempted to wrap undefined property getGeoHash as function
      at Context.<anonymous> (test/model-test.js:540:13)

  4) AGOL Model when building a geohash "after all" hook:
     TypeError: undefined is not a function
      at Context.<anonymous> (test/model-test.js:556:29)
chelm commented 9 years ago

@dmfenton remove your node_modules and reinstall...?

ungoldman commented 9 years ago

Tests are passing for me locally.

However, it is creating a file called ..2015-07-30 at the root of the repo with contents 2015-07-30T22:28:50.600Z debug {"status":202,"item":"itemid","layer":"0"}. This is using koop@2.6.0, so presumably the logger issue is not fully resolved. I'll try to get some tests around the logger itself and use the standard path module to resolve it once and for all tomorrow.

chelm commented 9 years ago

@ngoldman by specifying a path to the log dir in the model-test its creating the logfile. Other wise we could have the test write to the console (by omitting the path). I was just a bit annoyed by the logs when dumping them out to the console. I think the koop Logger is working as expected.

dmfenton commented 9 years ago

So the model still scares the hell out of me. But I still think we should merge this. Then we can:

chelm commented 9 years ago

@dmfenton The annoying thing is that we have to cut a release in order for it to get on QA

dmfenton commented 9 years ago

Not really, I can just point the package json at a git ref

chelm commented 9 years ago

@dmfenton true!

dmfenton commented 9 years ago

May god have mercy on our souls

ungoldman commented 9 years ago

:pray:

chelm commented 9 years ago

:godmode:

benheb commented 9 years ago

:clap: