Improved metrics for updating application #5607
Conversation
jansav
requested review from
nevalla and
jim-docker
and removed request for
a team
jansav
force-pushed
the
feature/improved-metrics-for-updating-application
branch
from
eef9344
to
58a322c
Compare
src/behaviours/application-update/analytics-for-installing-update.test.ts
Show resolved
Hide resolved
| */ | ||
| import moment from "moment"; | ||
|
|
||
| export const getCurrentDateTime = () => moment().utc().format(); |
There was a problem hiding this comment.
Shouldn't this be an injectable, so that you don't to do global.Data.now = ... in the tests.
There was a problem hiding this comment.
I'd like to say yes, but previous experience says that it's best to override Date by messing with globals, because of the all of the libraries that use it. If we used injectable for it, then we would have absolute control in our code base, but if (and when) we use some library that uses Date.now, we will get separate opinion which is easy way encounter hard-to-debug failing unit tests.
There was a problem hiding this comment.
Fair enough, thought after talking with @nevalla it seems that the timestamp is received automatically, so we don't need to include it.
There was a problem hiding this comment.
I'd say let's keep it there. Sometimes it's easier to access data from event itself, and not from the metadata. It all depends from the choice of service that we are using.
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
jansav
force-pushed
the
feature/improved-metrics-for-updating-application
branch
from
58a322c
to
84fc925
Compare
No description provided.