MAPD-597 Map Testability (2 of a few) #559
Conversation
- removed them as closures to drawFilterSummary to relieve memory pressure on the garabase collector. This has potential to be called often. Specs: Added - layerFormattersSpec for some basic tests Setting up some goals in specs so I can refactor some of the formatters to be controllers. (Maybe) re-enabling frontend coverage reports specs Trying to force istanbul via browserify to not count node_module deps and specs. We had false coverage numbers otherwise.
- specs - converted to service no longer needs mapCtrl as dependency
| @@ -23,4 +23,4 @@ CARTODB_API_KEY_TO_US=98d64004-be29-4f51-b883-1da478cf8f3d | |||
| # NEW_RELIC_API_KEY= | |||
There was a problem hiding this comment.
This will probably be contested, but this is not really an optional config and I would rather be able to set this in my rc file (.zshrc).
There was a problem hiding this comment.
If the root issue behind this is that you have nginx running already for other things, maybe we should just use a different port for our nginx instance so you can run both, and then we don't have to worry about handling 2 different types of dev environments.
I'd like to keep the amount of manual config (i.e. things people have to put in their rc files) to a minimum -- if I could figure out a good way to eliminate the need for the encryption key to be there, I'd be all for eliminating that as well.
There was a problem hiding this comment.
Does the new nginx instance continue to run even post running the script?
There was a problem hiding this comment.
No, it runs while the app runs, and shuts down when the app stops for any reason.
There was a problem hiding this comment.
Yes I want to hit nginx independent of the app. Why do I have to run a shell script to run everything. At times I want to only run foreman run coverageOpen which requires nginx to be running to serve the coverage pages.
I know we don't see eye to eye on this but I like fine grain control over my nginx. But I guess if I have to run the app and open coverage so be it.
There was a problem hiding this comment.
I'll revert the change and just deal with it.
There was a problem hiding this comment.
There's a lot of stuff that happens to ensure nginx starts and stops as part of the app (both on heroku and in dev). You can take a look here if you want to see it: https://github.com/realtymaps/nginx-buildpack/blob/master/bin/start-nginx
|
@zacronos .env reverted |
|
|
| .pipe open '', | ||
| url: 'http://localhost:3000/coverage/chrome/index.html' | ||
| .pipe open | ||
| uri: 'http://localhost:8085/coverage/application/index.html' |
There was a problem hiding this comment.
Can't you use a file:/// URI to open this as a local file rather than go through nginx?
There was a problem hiding this comment.
True; but this does help find issues with the wildcard logic. Also if we want to host the coverage so we can see it from heroku or wherever we need it on nginx.
There was a problem hiding this comment.
I'm not talking about removing the file from a path where it can be served by nginx, just using a local URI in this gulp task when it automatically pops open a window with the report. (That only happens on dev.)
When nginx is running, you could still browse to http://localhost:8085/coverage/application/index.html and get the report. The file will also still be available on heroku through nginx, and thus can still be used as a test for the wildcard -- but you're also free to run the task without nginx, like you wanted. It's best of both worlds.
I'll go ahead and merge, you can make that change (if you want) in the next PR.
…ility MAPD-597 Map Testability (2 of a few)
No description provided.