search

AgileVentures / projectscope

MVP dashboard for ProjectScope, using new gems architecture developed by AV folks
2 stars 14 forks source link

EPIC/timeslider #66

Closed junyu-w closed 1 year ago

junyu-w commented 7 years ago

PT:

  1. https://www.pivotaltracker.com/story/show/134241103
  2. https://www.pivotaltracker.com/story/show/134241103

Features:

  1. added timeslider to check previous metric samples
tansaku commented 7 years ago

@DrakeW needs merge

tansaku commented 7 years ago

@DrakeW merge conflicts, and maybe need some jasmine tests for this new javascript functionality?

junyu-w commented 7 years ago

@tansaku looks like the jasmine test messed up the CI setting somehow that it could never quit the jasmine server... couldn't find any useful information about the error happened on CI regarding some glyphicon missing in asset pipeline..

junyu-w commented 7 years ago

@tansaku looks like this is a known issue to the gem due to its incompatibility with sprockets-rails. https://github.com/jasmine/jasmine-gem/issues/275

I will just remove jasmine test from CI build but still keep the test files so that developer can test locally

what do you think?

tansaku commented 7 years ago

@DrakeW reasonable first pass - I will take a more detailed look - sorry for slow response

tansaku commented 7 years ago

little confused about why we have two PT links to the same thing :-) any PR should ideally be related to just one PT item to ensure focus - can you review?

tansaku commented 7 years ago

@DrakeW I can't run jasmine locally due to this error:

[2016-11-23 18:49:42] ERROR Sprockets::Rails::Helper::AssetNotFound: The asset "bootstrap/glyphicons-halflings-regular.eot" is not present in the asset pipeline.

is this the same you were getting on CI?

junyu-w commented 7 years ago

@tansaku sorry for the late reply, just got back from thanksgiving break

and yes that was the same error on CI and those two PT links were front-end and back-end of this epic, but later I found that that are kind of tightly related not not too much work either, so I just did both in one PR

junyu-w commented 7 years ago

@tansaku is this feature ready to be merged in?

tansaku commented 7 years ago

@DrakeW getting there - great to see the jasmine tests in there - I will review more thoroughly now

tansaku commented 7 years ago

so @DrakeW I get this error when I run locally:

The asset "bootstrap/glyphicons-halflings-regular.eot" is not present in the asset pipeline.

you don't.

I'm thinking about the long term maintenance of the app, and I'd really like to see the jasmine tests running on my machine and on the CI

tansaku commented 7 years ago

I get it trying to run the server (i.e. not ci mode):

same error in local server

tansaku commented 7 years ago

I see you posted the error to the jasmine folks: https://github.com/jasmine/jasmine-gem/issues/275 well done

tansaku commented 7 years ago

@DrakeW I rolled back to an older version of sprockets and the jasmine tests work now

tansaku commented 7 years ago

looking more closely at the specs, I see we have six new js functions, and one jasmine test ... would we perhaps ideally have one spec per js function?

junyu-w commented 7 years ago

@tansaku added more jasmine tests and back to CI, and all are passing now :)

tansaku commented 7 years ago

Great to see it green @DrakeW - should we prefer to download jasmine-jquery off a CDN rather than checking it into our codebase?