Demonstrate feasibility of change freq after instance creation #855
Conversation
| set: function(newFreq) { | ||
| priv.set(this, { | ||
| freq: newFreq | ||
| }); |
There was a problem hiding this comment.
This will pave over the existing state object.
Edit:
One you make the change above, to the state object, this can be: state.freq = newFreq;
There was a problem hiding this comment.
I have not worked with javascript Map previously. Transposing from other languages, I was thinking that 'this' was a context, not a key. Instead, priv is a file scope variable (class static in other languages), and the instance ('this') is the key. I actually implemented a similar structure in older (es5) javascript using a serial number incremented by the constructor as a key to create class and instance private storage. Map and using a reference to the instance object makes it cleaner. Time to write a test function, to make sure I get the capabilities straight.
Since the existing unittests did not notice this, I also need to figure out what should be there to catch that.
There was a problem hiding this comment.
Since the existing unittests did not notice this, I also need to figure out what should be there to catch that.
After setting sensor.freq the values returned by any other getter would've been undefined—that might be useful for ensuring integrity of private data
There was a problem hiding this comment.
Turns out that with the existing code, checking for that case is not easy. The new freq get function is the only one that explicitly returns the private state value. Everything else just uses the value as part of some calculation. The best check I could come up with was to inject a value through analogRead, use booleanAt to set a barrier value that returns true for the value, then check that boolean was still true after setting the new freq property value. Wiping out the booleanBarrier state value cased boolean to go from true to false. Messy, and an incomplete check.
Is it appropriate to add a readonly state property to return the complete contents of the private data? That could then be easily added to getShape() function I added for checking property changes. Alternatively, and potentially more appropriate, defined property get functions could be created for each of the private state properties. Something like: barrier, scale, val. For my (testing) purposes, a read only state property would be easiest to use to provide full coverage. The deepEqual check would automatically catch any unexpected changes. New additions to the state properties would only need to be added to the defShape object to be fully included in all of the instance property validation done by the deepEqual tests . That is actually what the shape checks were intended to catch. Any unanticipated side effects. But as is, it does not see changes in the instance private state properties.
There was a problem hiding this comment.
How about:
var IS_TEST_MODE = !!process.env.IS_TEST_MODE;
// ...
if (IS_TEST_MODE) {
Object.defineProperties(this, {
state: {
get: function() {
return priv.get(this);
}
}
});
}so the private information is only exposed for testing?
There was a problem hiding this comment.
so the private information is only exposed for testing?
This works for me
|
On the whole, this is a sound approach. I'm glad it didn't turn out too gnarly. My one bit of critical feedback is that the test changes are a bit a dense and the result is overwhelming. I'm having a hard time following what is being tested and how—it's not clear at a glance. I'll spend more time with it. |
|
From the above comment, I need to add purpose / description comments to the tests. As well as other things mentioned. I DID start by saying this was pretty raw, though I was thinking more about the code than the tests. |
Sounds good :) |
|
Unposted changes are looking pretty good. Just going back to add better documentation for the test cases, then I can commit and update the PR. |
lib/sensor.js Move median calculation function definition outside of the constructor function Move latest median value storage to closure scope, to provide history between calls Add read only, test mode only, state property to expose internal instance state information Use a local (constructor function scope) variable to access the private state properties Replace (in constructor) references to the private state information with the local variable test/sensor.js Add test cases for freq, threshold, id, limit properties Extend expected instance shape information Expand testing for analog noise filtering Expand testing for data and change events Remove some duplicate tests (data) Streamline instance shape tests using deep object compare Require loadah module to get access to cloneDeep method Add function to create object containing the current instance shape information Fix test for unexpected properties in sensor instance Add comments to document the test sequences Add closing comment for function definitions Add some line breaks for readability, and lint nit pick.
… to i2cConfig This is needed by platforms that have > 1 active I2C bus
Add Micro Magician examples and Shield Config
|
PR branch updated earlier in the week. Anyone had a look at the changes? |
|
@mMerlin bah, github doesn't ping when a PR is updated—so I had no idea :\ I will look at this weekend! |
|
I did a bunch of cleanups throughout the tests, making sure board instances were disposed of in all |
lib/sensor.js Move median calculation function definition outside of the constructor function Move latest median value storage to closure scope, to provide history between calls Add read only, test mode only, state property to expose internal instance state information Use a local (constructor function scope) variable to access the private state properties Replace (in constructor) references to the private state information with the local variable test/sensor.js Add test cases for freq, threshold, id, limit properties Extend expected instance shape information Expand testing for analog noise filtering Expand testing for data and change events Remove some duplicate tests (data) Streamline instance shape tests using deep object compare Require loadah module to get access to cloneDeep method Add function to create object containing the current instance shape information Fix test for unexpected properties in sensor instance Add comments to document the test sequences Add closing comment for function definitions Add some line breaks for readability, and lint nit pick.
|
I did the rebase. It was not as simple as implied. Had to resolved several merge conflicts, after looking at the current sources you committed. The merge made a mess in a few places, inserting my new function in the middle on one of your new functions. Everything seems to be resolved now, but the PR does not look good. It is showing 48 file changes, not just the 2 I changed. The others seem to be the things you already did in the test refactoring. I don't know what I/git did to cause that. Looks like I may need to save the 2 changed files, delete the PR, refresh my branch, re apply the changes, and recreate the PR. Confirm? Other options? Can I (and how) delete the pull request, or do you need to do it? Got outside the places I have worked previously. <grin> again </grin> |
|
This looks like a merge, not a rebase. I will try to resolve it for you tomorrow. |
|
It started as a rebase, which said it was doing a rollback and replay. Automatic merge failed on replay, and instructions said to fix that and continue. I did, then tried to push. That told me I needed to do a pull first, which generated more manual merges, before I could commit and push. |
This is the culprit, next time just force push your branch. |
|
Also, I see the function issue you mentioned |
|
Landed a79f247 |
This is a raw implementation. I recommend NOT merging this until it has been cleaned up a bit. This is just an easy way to show that changing the freq property, and associated event intervals is not that difficult. Extending from conversation in #854.
The goal of this implementation was to minimize the changes shown by git diff while getting the added functionality to work.