GPII-4468: WIP - system now reacts propertly to /enabled: false through not starting solution #883

amb26 · 2020-06-17T23:00:48Z

repaired a few tests, some remain, including Journal tests and magnifier portion of integration tests

…gh not starting solution - repaired a few tests, some remain, including Journal tests and magnifier portion of integration tests

amb26 · 2020-06-17T23:02:29Z

@the-t-in-rtf , @sgithens - the status of this is -

I've verified that logging is as carla now correctly does not start the magnifier but does configure it
I've fixed up a few tests, but a few are still broken -
-- JournalIntegrationTests are commented out
-- Each acceptance tests currently gets one failure because the magnifier running state is not reset on logout
But hopefully this is good enough to get unjammed with work such as Fusion onboarding and general capture stuff

gpii-bot · 2020-06-17T23:35:36Z

CI job failed: https://ci.gpii.net/job/universal-tests/2378/

the-t-in-rtf · 2020-06-18T08:06:21Z

Each acceptance tests currently gets one failure because the magnifier running state is not reset on logout.

Just to confirm, you mean that the magnifier (or any /enabled solution) is not stopped on key out? That was the third problem we discussed in our call.

the-t-in-rtf · 2020-06-18T08:07:05Z

gpii/node_modules/lifecycleManager/src/LifecycleManager.js

@@ -1141,7 +1141,7 @@ var fluid = fluid || require("infusion"),
            if (desiredRunState === true) { // and it was running when we started
                actions = [ configurationType, "start" ];
            } else { // just restore settings
-                actions = [ "restore" ];
+                actions = [ "restore" ]; // TODO: this should probably be "update" too


Maybe just a word or two about why should it be "update"?

the-t-in-rtf · 2020-06-18T08:23:34Z

testData/preferences/explodeLaunchHandlerStart.json5

@@ -5,6 +5,7 @@
            "gpii-default": {
                "name": "Default preferences",
                "preferences": {
+                    "http://registry.gpii.net/applications/net.gpii.explode/enabled": true,


Does this have to be expressed as a full URL rather than as a property under net.gpii.explode?

Looks like it does. I wish it didn't, it means all of the /enabled stuff is much more difficult to validate. It also means we have no good way of indicating which things can be enabled at all, i.e. we can pass along any solution's URL with /enabled at the end whether or not it's meaningful.

OK, we could investigate how much rework would be required to bundle this in as a standard setting. As I recall, we didn't want to pollute the existing namespace of settings in case that name was used by something else - but I guess transforms are sufficiently mature that these could easily be diverted. Historically, I think that "/enabled" was an initial attempt to clean up what had been pretty irregular URLs under the old "common terms" system such as /magnifierEnabled

If it's too much work to make this work like any other setting we should at least discuss whether/how we can clearly indicate which things can be /enabled and add detection. i.e. if what happens if someone tries to enable something that is always running and which cannot be stopped? What if they pass /enabled for a solution that has no launch handlers at all?

the-t-in-rtf · 2020-06-18T09:40:35Z

Just to confirm, you mean that the magnifier (or any /enabled solution) is not stopped on key out? That was the third problem we discussed in our call.

Just confirmed that /enabled works only for starting, as I more or less expected based on having to close the magnifier, et cetera after every acceptance test run.

amb26 · 2020-06-18T09:55:26Z

I didn't recall we had already found a /enabled problem on key out - hopefully it has a common cause and doesn't represent a 4th problem. Could you just walk me through in detail how far we had characterised it by the time of the call?
I had noticed for a long time that the magnifier was left running after acceptance tests runs but had assumed it was just some insufferable race condition whereby the component tree was torn down early before things finished being stopped - since it didn't actually cause the tests to fail. However, now we have the situation that even the universal tests fail because the system apparently does not even try to stop the solution - or on the other hand, it might be that the mocks are faulty.
For example, for "carla", the magnifier was stopped properly on keyout - but, of course, that prefs set did not encode "/enabled".

the-t-in-rtf · 2020-06-18T12:36:15Z

It's easy to see if you have JAWS installed, if you key in using the stock "jaws" user, JAWS is definitely launched, when you key out, it's not stopped. This obviously makes it difficult to test the "stop" configuration for Fusion 2020.

Not sure if it works, but testData/preferences/GPII-270-rbmm-demo.json5 enables high contrast, which you might be able to use to test without installing anything else.

the-t-in-rtf · 2020-06-18T12:48:06Z

For example, for "carla", the magnifier was stopped properly on keyout

It doesn't start or stop when I use your branch in my VM.

amb26 · 2020-06-18T12:51:26Z

It doesn't start or stop when I use your branch in my VM.

The version in the branch doesn't have "/enabled" and so currently just gets configured. I was speaking of the situation in the past - since this could have been all you were referring to in our Tuesday meeting when you said you had raised 3 issues.

the-t-in-rtf · 2020-06-18T12:56:51Z

Ah, so you meant "was stopped" because it had settings and we were running with the old behaviour. Got it.

Just to be clear about which three I thought we had discussed:

If there were settings for a solution, it was launched. (you've fixed that)
If you only passed /enabled, the solution was not launched. (you've fixed that)
If the solution was launched because it had settings, it wasn't stopped on keyout. (still seems to be an issue)

…failure

amb26 · 2020-06-18T22:01:23Z

Hi there @the-t-in-rtf - so far I believe I have only fixed point 1 of the 3. I will look at point 3 next, but in the meantime I wonder if you could take a look at the remaining test failure in this branch which is caused by a validation failure when I introduce a capabilities block into the Zoomtext solution. I'm puzzled by it since this block is identical to that in other solutions, e.g. Windows magnifier, and the text of the message is rather confusing and doesn't explain what rule is violated. Here it is:

22:47:29.702:  Validation error in solution file 'win32.json5' -> solution 'com.aisquared.zoomtext':
22:47:29.702:    - capabilities.0]: The supplied string does not match the expected pattern.
22:47:29.703:  jq: FAIL: Module "Sanity checks for Solutions Registry files." Test name "Sanity-checking solution 'com.aisquared.zoomtext' in file 'win32.json5'." - Message: Solution 'com.aisquared.zoomtext' in file 'win32.json5' should be valid according to the schema.

gpii-bot · 2020-06-18T22:30:42Z

CI job failed: https://ci.gpii.net/job/universal-tests/2381/

the-t-in-rtf · 2020-06-19T12:23:57Z

So, it's trying to tell you that the first capability doesn't match the pattern defined in the schema for a solution. It will pass if you escape the dots like all but two of the entries in win32.json5. I can improve that pattern if needed, but it would be good to confirm whether the escaping used throughout the file is necessary.

amb26 · 2020-06-24T12:48:57Z

Thanks, that's a good catch - in fact every one of the other entries in win32.json seem to be escaped, which two were you thinking of?
It's puzzling actually in the codebase where, if anywhere, the de-escaping of capabilities takes place, I will need to check this since wherever we perform computations on them they seem to appear in unescaped form like so: https://github.com/GPII/universal/blob/master/gpii/node_modules/matchMakerFramework/src/MatchMakerUtilities.js#L442

the-t-in-rtf · 2020-06-25T09:43:42Z

The ones that had errors reported, such as:

22:47:29.702:  Validation error in solution file 'win32.json5' -> solution 'com.aisquared.zoomtext':
22:47:29.702:    - capabilities.0]: The supplied string does not match the expected pattern.

The pathing is broken over two lines, but that should be enough to let you find the two needles in the haystack.

…verify shutting down

gpii-bot · 2020-06-29T16:00:37Z

CI job passed: https://ci.gpii.net/job/universal-tests/2386/

… an application-specific "/enabled" term

gpii-bot · 2020-06-30T15:36:06Z

CI job passed: https://ci.gpii.net/job/universal-tests/2387/

amb26 · 2020-06-30T15:59:22Z

This pull request should probably never be merged since it lacks adequate tests to cover the changed functionality, and has had a few lifecycleManager tests commented out as a result of their seemingly redundant use of "active: true" as an alternative means of expressing that a solution should be started. However, it should be good enough to support any remaining manual testing work and of course we could roll a dev release from it if required. There are two further pref sets, carla2 and carla3 which can be used to manually verify that the previously faulty cases with respect to /enabled have been fixed.

GPII-4468: WIP - system now reacts propertly to /enabled: false throu…

6a42a55

…gh not starting solution - repaired a few tests, some remain, including Journal tests and magnifier portion of integration tests

the-t-in-rtf reviewed Jun 18, 2020

View reviewed changes

GPII-4468: Resolved all test failures except for solution validation …

4e12308

…failure

GPII-4468: Fixed test cases for new "/enabled" model - still need to …

e3271ff

…verify shutting down

GPII-4468: Fix for failure to select/start application which only has…

0f13cda

… an application-specific "/enabled" term

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPII-4468: WIP - system now reacts propertly to /enabled: false through not starting solution #883

GPII-4468: WIP - system now reacts propertly to /enabled: false through not starting solution #883

amb26 commented Jun 17, 2020

amb26 commented Jun 17, 2020

gpii-bot commented Jun 17, 2020

the-t-in-rtf commented Jun 18, 2020

the-t-in-rtf Jun 18, 2020

the-t-in-rtf Jun 18, 2020

the-t-in-rtf Jun 18, 2020

amb26 Jun 18, 2020

the-t-in-rtf Jun 18, 2020 •

edited

Loading

the-t-in-rtf commented Jun 18, 2020

amb26 commented Jun 18, 2020

the-t-in-rtf commented Jun 18, 2020

the-t-in-rtf commented Jun 18, 2020

amb26 commented Jun 18, 2020 •

edited

Loading

the-t-in-rtf commented Jun 18, 2020

amb26 commented Jun 18, 2020

gpii-bot commented Jun 18, 2020

the-t-in-rtf commented Jun 19, 2020

amb26 commented Jun 24, 2020

the-t-in-rtf commented Jun 25, 2020

gpii-bot commented Jun 29, 2020

gpii-bot commented Jun 30, 2020

amb26 commented Jun 30, 2020

GPII-4468: WIP - system now reacts propertly to /enabled: false through not starting solution #883

Are you sure you want to change the base?

GPII-4468: WIP - system now reacts propertly to /enabled: false through not starting solution #883

Conversation

amb26 commented Jun 17, 2020

amb26 commented Jun 17, 2020

gpii-bot commented Jun 17, 2020

the-t-in-rtf commented Jun 18, 2020

the-t-in-rtf Jun 18, 2020

Choose a reason for hiding this comment

the-t-in-rtf Jun 18, 2020

Choose a reason for hiding this comment

the-t-in-rtf Jun 18, 2020

Choose a reason for hiding this comment

amb26 Jun 18, 2020

Choose a reason for hiding this comment

the-t-in-rtf Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

the-t-in-rtf commented Jun 18, 2020

amb26 commented Jun 18, 2020

the-t-in-rtf commented Jun 18, 2020

the-t-in-rtf commented Jun 18, 2020

amb26 commented Jun 18, 2020 • edited Loading

the-t-in-rtf commented Jun 18, 2020

amb26 commented Jun 18, 2020

gpii-bot commented Jun 18, 2020

the-t-in-rtf commented Jun 19, 2020

amb26 commented Jun 24, 2020

the-t-in-rtf commented Jun 25, 2020

gpii-bot commented Jun 29, 2020

gpii-bot commented Jun 30, 2020

amb26 commented Jun 30, 2020

the-t-in-rtf Jun 18, 2020 •

edited

Loading

amb26 commented Jun 18, 2020 •

edited

Loading