GPII-228 Capture Settings Backend (Snapshotter 2018) #616
Conversation
- Next step will be to fix issues with the SPI Settings Handlers, and then finish revising the payload that will need to be returned.
sgithens
changed the title
|
CI job passed: https://ci.gpii.net/job/universal-tests/891/ |
|
CI job failed: https://ci.gpii.net/job/universal-tests/1422/ |
|
CI job failed: https://ci.gpii.net/job/universal-tests/1423/ |
|
CI job failed: https://ci.gpii.net/job/universal-tests/1440/ |
|
CI job failed: https://ci.gpii.net/job/universal-tests/1634/ |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2118/ |
|
I made a minor change so that the tests pass now. (previously it was using the actual live platform reporter). This should still actually work fine without the world destroying GPII-3119, since the SPI settings workaround is still in the lifecycle manager. Whenever GPII-3119 is done, that workaround can simply be removed. |
| ); | ||
| }; | ||
|
|
||
| gpii.flowManager.getSolutionsPromise = function (solutionsRegistryDataSource, deviceContext) { |
There was a problem hiding this comment.
Rather than adding a new getSolutionsPromise function, have you tried to get getSolutions return a promise besides firing events and have the capture component call getSolutions?
There was a problem hiding this comment.
Neat idea, I updated the function to handle both purposes, and cleaned up it's jsdoc
|
|
||
| // The solutions block is a read only data structure | ||
| var adjustedSolutions = fluid.copy(solutionsRegistryDataSource.fullSolutionsRegistry); | ||
| adjustedSolutions.darwin.fakemag2.settingsHandlers.configuration.options.filename = testDataFile; |
There was a problem hiding this comment.
When all settings handlers consume same set of settings/values, it would be hard to detect a coding error that causes the same data being returned.
There was a problem hiding this comment.
this is a really good point. I created a second file, and added a few settings and make some subtle changes to the types.
| * Tests for formatting raw captures. | ||
| */ | ||
| gpii.tests.flowManager.capture.testFormatRawCapturedSettings = function () { | ||
| var allCorrectSettings = gpii.flowManager.formatRawCapturedSettings([ |
There was a problem hiding this comment.
A couple of thoughts:
- It would be easier to read and maintain if all test cases are collected together. Just an example:
var testCases = {
"allCorrectSettings": {
message: "...",
input: {...},
expected: {...}
},
"withFetchError": {
[same structure as above]
},
...
};
fluid.each(testCases, function (oneTest) {
var result = gpii.flowManager.formatRawCapturedSettings(oneTest.input);
jqUnit.assert(oneTest.message, oneTest.expected, result);
});
- Please also test edge cases such as, no settings are returned, all fetch errors etc.
- Please use a slightly different setting sets/values especially when multiple settings are tested in one single test case so it doesn't miss the coding error when the same result is returned for all settings.
There was a problem hiding this comment.
Reworked these, and added more edge cases.
- Updated method name packages - Updated license header - Removed unused code - Converted single line methods to invoker definitions - Used fluid.get for property access
|
CI job passed: https://ci.gpii.net/job/universal-tests/2133/ |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2134/ |
- Updated jsdoc formatting - Updated event names - Restructured tests and added more
|
CI job passed: https://ci.gpii.net/job/universal-tests/2136/ |
Updated these names |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2137/ |
| }, | ||
| getSystemSettingsCapture: { | ||
| funcName: "fluid.promise.fireTransformEvent", | ||
| args: ["{that}.events.onCaptureSettingsForCurrentDevice", {}, "{arguments}.0"] // options |
There was a problem hiding this comment.
It's confusing to have the second argument as an empty object. BTW, you didn't document it in the jsdoc for this function. I understand you use it as a placeholder for the solution list returned by getInstalledSolutionsForCurrentDevice(). But this gonna to be expressed in a more understandable way. What I can think of is:
- remove this argument;
- improve the first action
onCaptureSettingsForCurrentDevice.getInstalledSolutionsForCurrentDevicein the promise chain to return:
{
installedSolutions: {},
options: {}
}
- This structure will be the input of the next listener.
There was a problem hiding this comment.
Well, you did have a doc for this empty object although it's inaccurate:
* @param {Object} solutions - Solutions registry entries for solutions available on the current machine.
"solutions available on the current machine" is a value waiting to be calculated within this function rather than an empty object.
There was a problem hiding this comment.
Ok, so I cleaned things up abit. I also removed another internal invoker that was just for the promise chain and declared it in line. The remaining two invokers are both for external usage, and I added a jsdoc for the first one as well.
I cleaned up the arguments on the other jsdoc... actually it had a couple arguments that were no longer relevant anymore, it only takes the one single options argument. It was still documenting the parameters from the function version rather than the invoker setup.
I changed the transforming promise chain invocation's argument from an empty object to just null, to hopefully make it more clear. It is only there so I can use the 3rd argument from the fire transform api. https://github.com/fluid-project/infusion-docs/blob/master/src/documents/PromisesAPI.md#fluidpromisefiretransformeventevent-payload-options
I'd like to keep the promise chains set up the way they are if possible, and keep the options in the designated 3rd parameter that each of the listeners can optionally except. I think it's pretty nice that each member in the chain takes the previous value and returns the next promise with no extra stuff. It's just that the promise chain doesn't require any payload to start, which is which is why the null is there.
|
CI job passed: https://ci.gpii.net/job/universal-tests/2141/ |
|
This pull request looks good to me. Thanks, @sgithens. |
4 participants
https://issues.gpii.net/browse/GPII-228
This is in progress work of resurrecting @kaspermarkus 2014 work on a snapshotting tool. (Now being called capture since we have some other core utilities that have been developed in the interim that are named snapshotting for journalling)
This still requires some work on the SPI settings handler, and settings handlers in general, but I would welcome any comments on the work thus far.