feat: Add StepHistory class
#88
Conversation
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
==========================================
+ Coverage 90.14% 90.31% +0.17%
==========================================
Files 10 11 +1
Lines 284 289 +5
==========================================
+ Hits 256 261 +5
Misses 28 28
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@pgrzesik thanks for that. Still why do you think we need this to be introduced here?
I imagine that we will inject stepHistory instance in all cases here: https://github.com/serverless/serverless/blob/f4c9b58b10a45ae342934e9a61dcdea0c2ef11e2/lib/cli/interactive-setup/index.js#L19
And there will be no other places where this instance will be created (?)
|
It is needed in order to properly test steps in |
And how exactly do you imagine testing this (?) I think at this level testing existence of expect(Array.from(stepHistory).map(([key, value]) => [key, value.value])).to.deep.equal([
['orgName', '_user_provided_'],
['appName', '_user_provided_']
]);We can also add helper on expect(stepHistory.valuesMap).to.deep.equal(new Map([
['orgName', '_user_provided_'],
['appName', '_user_provided_']
])); |
There was a problem hiding this comment.
After discussing it internally, all clear. I missed that we inject StepHistory instances to mocked context object.
Can we also have some sanity test? That adds few items and confirms toJSON output?
Also maybe it's worth to add valuesOnlyMap() method that could aid testing with values stripped of timestamps
telemetry.js
Outdated
| class StepHistory extends Map { | ||
| set(key, value) { | ||
| const valueWithTimestamp = { | ||
| ...value, |
There was a problem hiding this comment.
Shouldn't it be value: value ?
There was a problem hiding this comment.
I've originally proposed an approach where the value is an object, but it's not used at the moment and I guess it will be better to stick to such approach
There was a problem hiding this comment.
Even if value is an object, I would still then store this value at value property.
At least top level should be just meta properties, and I think at this stage we do not see the use case for others (if there will be for ones to be injected with set may consider supporting them with third argument)
pgrzesik
force-pushed
the
add-step-history-class
branch
2 times, most recently
from
45e876d
to
0c4b1af
Compare
telemetry.js
Outdated
| set(key, value) { | ||
| const valueWithTimestamp = { | ||
| value, | ||
| timestamp: Math.round(Date.now() / 1000), |
There was a problem hiding this comment.
changed - I took inspiration from how we record first installation of the Framework where we strip it to seconds, but here it makes more sense to have milliseconds
telemetry.js
Outdated
| } | ||
|
|
||
| toJSON() { | ||
| return Array.from(this.entries()).map(([key, obj]) => ({ question: key, ...obj })); |
There was a problem hiding this comment.
If we rename key to question. Shouldn't we rename value to answer (?)
I'm fine with both key/value and question/answer pairs. but I'm not sure about question/value pair.
Also, while we do not have such case now, key/value may play better, if we decide to rely on similar internal tracking with some other command. (e.g. imagine tracking packaging, first generating archives and then uploads, reporting it with question/answer feels bad)
pgrzesik
force-pushed
the
add-step-history-class
branch
from
0c4b1af
to
bf8e142
Compare
Add
StepHistoryclass as it has to be reused betweendashboard-pluginandserverless