This repository has been archived by the owner on Jun 28, 2022. It is now read-only.
Bug 1128988 (part 2) #77
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a simplification I've wanted to do for ages, those functions form part of the front interface so we really should get it right before we start using it in anger. Signed-off-by: Joe Walker <[email protected]>
The confusion was causing problems, so it seems simplest to fix it. Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
More testing of async as a result of the use in Firefox has flushed out some cases that were not correct. Signed-off-by: Joe Walker <[email protected]>
The server sends a block of JSON to the client describing the commands and on the client the parameter types are turned into proper Type objects and the 'front' object injected, but this can only happen if the spec is an object rather than just a string. The remote type should be able to remote itself trivially. Signed-off-by: Joe Walker <[email protected]>
I'm not clear why we thought that being able to display json was only something you'd want to do during testing, but since we later added them to the default set of converters, we don't need duplicates in test code. Signed-off-by: Joe Walker <[email protected]>
Most of the test commands had an exec function generated by 'createExec' which previously outputted (synchronously) a block of text that described the arguments (using toString) This had the problem that we couldn't test the values individually, so now createExec returns a testCommandOutput structure containing the correct argument stringified version (when promises). A number of tests change to reflect this, and it needs an easy way to lookup parameters by name in Command. Signed-off-by: Joe Walker <[email protected]>
io.js uses the full path to node in process.title. Signed-off-by: Joe Walker <[email protected]>
We had various hacky ways to mock and setup the document against which the 'node' and 'resource' types acted, so we could run tests against NodeJS, the web, the web remoted to NodeJS and Firefox. With this change, all tests assume that the environment is setup correctly so there is some sort of a DOM. We remove the hacky fake DOM and replace it with a less hacky DOM from jsdom. Signed-off-by: Joe Walker <[email protected]>
Better error message, and make sure we don't swallow the promise rejection. Signed-off-by: Joe Walker <[email protected]>
For the remote case, we're calling "mocks on" on the server, but we still need to register the mockCommand converters on the client. Signed-off-by: Joe Walker <[email protected]>
The majority of the isNoDom checks are now irrelevant because there is always a DOM that works for us to test against. Signed-off-by: Joe Walker <[email protected]>
This is mostly tweaks to tests. The node type doesn't call createEmptyNodeList where it doesn't need to, and the NodeList type now sets up the ability to convert back to a string properly so the remote tests can at least prove that the value (which isn't available remotely) could be stringified into something expected. Signed-off-by: Joe Walker <[email protected]>
Several tests were skipped when there wasn't a DOM to test against. This removes all those test skips. Given the recent fixes, most of the skips could just be removed. Some of the skips are not down to weaknesses in jsdom, but in NodeJS or the requisitionAutomator which is used only in NodeJS, so some of this is s/isNoDom/isNode. Some of the problems were confusion as to an id in the document that we were testing against. #gcli-input was removed some time ago, so we're now using #gcli-root which still exists. Finally I added a trivial converter for the 'pref list' command so it would work on the command line, and be tested properly. Signed-off-by: Joe Walker <[email protected]>
Fix for joewalker/gecko-dev@2a963b0 Signed-off-by: Joe Walker <[email protected]>
Fix for bc82016#commitcomment-10060854 Signed-off-by: Joe Walker <[email protected]>
Repository owner
locked and limited conversation to collaborators
Mar 9, 2015
Previously each time we navigated to a new page we'd have to update the document/global in GCLI. A windowHolder which uses a getter property to dynamically return the correct document/global is much easier to use. We're using windowHolder rather than documentHolder simply because you could confuse a documentHolder and a window, but you can't confuse a windowHolder and a document so easily. The javascript type isn't used in Firefox. It wasn't clear that the donteval test was working properly, and I didn't want to invest time to it right now, so we're skipping those tests. Signed-off-by: Joe Walker <[email protected]>
Repository owner
unlocked this conversation
Mar 11, 2015
In Output.toJson we were assuming that `JSON.stringify(new Error('x'))`
would give us something useful. It actually just gives `{}`, so we
manually extract the properties of a standard Error.
We were using `error:true` to denote an error, but this was conflicting
with a special property in protocol.js, so we're using isError instead.
(Commands that fail are considered non-exceptional. Exceptions in GCLI
are for when GCLI itself fails)
The converters now need to cope with errors that are objects that look
like Errors (but not actually errors).
In the process of debugging this I refactored the code that kicks off
a remote execution in system.js.
Signed-off-by: Joe Walker <[email protected]>
2 Changes: - s/*function/function*/ - Use a <div> rather than a <p> (some tests assume this) Signed-off-by: Joe Walker <[email protected]>
Someone had added an assert.ok, and we protect ourselves against null DOM nodes in checking test output Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
The whole onFailure thing wasn't needed, and we can chain the promises onto each other rather than manually running a loop. Half the LOC. Signed-off-by: Joe Walker <[email protected]>
delegate.stringify does something better than die if it gets undefined. We url.parse checks if environment.window exists without accessing it. Signed-off-by: Joe Walker <[email protected]>
Firstly the nodelist type tried to instansiate itself across the remote
interface, which was never going to work. Now it uses the 'remote' type
to proxy the type functions.
There is a special 'blank' conversion for types. parse() is async but
lots of the time we're effectively doing parse('') and making that case
async spreads the async virus to lots of places that it doesn't need to
be. So there's a getBlank() shortcut that is sync.
In almost all cases blank means 'INCOMPLETE' with an undefined value
(see [1] for the default implementation). But there are exceptions like
nodelist, where blank means an empty NodeList (which could be VALID)
So we add a blankIsValid property to the remote type which defaults to
false, so you get the default getBlank() behavior. But for nodelists
we set this to true, so we get the correct VALID / empty NodeList
behavior.
[1]: https://github.com/joewalker/gcli/blob/585f7b83953de5c0faeb9fb187b55ab6171c3d03/lib/gcli/types/types.js#L1007
Signed-off-by: Joe Walker <[email protected]>
nodelist types have a blank value that is an empty NodeList, and a NodeList is tied to a document which can change (by navigation) so we need to ask call getBlank() whenever we want the defaultValue. Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
I'm not sure we'll ever actually need to double-remote a type, but just in case we should pass on the data that's needed. Signed-off-by: Joe Walker <[email protected]>
When blank isn't valid it's important that we have a value of undefined so that the isDataRequired calculation works, which seems broken. I'm not going to dig deeply into fixing it now though. Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
I'm really surprised that this wasn't there already. Signed-off-by: Joe Walker <[email protected]>
We made this change a while back for modules that were loaded without delay. This does the same for modules with delayedLoad:true Signed-off-by: Joe Walker <[email protected]>
We're going to load mockCommands into the server process which means it needs to be loadable via require as well as loadScript. We also want to minimize the processing done as we sync the file between trees. Signed-off-by: Joe Walker <[email protected]>
And not camelCase. Also we're not sending the specs with the event because then we'd need to get customProps right, and because it prevents the receiver deciding if it wants to transfer a significant amount of data. Signed-off-by: Joe Walker <[email protected]>
We don't want to light up the remote interface 10x for every module, so this adds holdFire/resumeFire to addItemsByModule and friends. Signed-off-by: Joe Walker <[email protected]>
We're (broadly) doing 2 things here. Partly we're causing all mock commands to be loaded into the server process. We're still running the test commands against the client process. This exercises the more of the infrastructure with the test suite. The changes to do that double the size of the test runner that we inject into all the unit test files. Cut and paste is bad, so we abstract this into a new function in helpers.js This change fixes how we alter the test files as we copy them from the GCLI tree to mozilla-central. It's probably best to understand this looking at what happens to central when we run this command. Signed-off-by: Joe Walker <[email protected]>
Minor changes aimed at fixing the output of the previous commit. Again this probably makes more sense looking at the results, for which, see "The results of running 'firefox ../gecko-dev'" (joewalker/gecko-dev@3915d17) in joewalker/gecko-dev#10 Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
Selection types have 2 ways to set the available options. 'data' allows you to provide an array of strings for types when the underlying type is a string. 'lookup' allows you to provide an array of objects with name and value properties for times when the underlying type isn't a string. Previously we had 2 remote functions one for data and one for lookup. The selection type has a function (getLookup) which does the work of resolving promises, resolving cases where data or lookup is a function and converting from data to lookup. There wasn't much point in having 2 remote functions when we could just use getLookup to resolve both. The changes to remoted.js will need to be mirrored in gcli.js in Firefox. Signed-off-by: Joe Walker <[email protected]>
Can't transfer the global remotely. Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
Lots of JS tests affected by how it's remoted, and the skips in testJs were on crack, but that's unused in Firefox so we're not too worried about that. Resource tests are now run remotely too, and we can't run tests locally on firefox. The new command test helps with that. Signed-off-by: Joe Walker <[email protected]>
Clean up worked while things were less async. Then it was an accident waiting to happen. Signed-off-by: Joe Walker <[email protected]>
Just go through the loaded modules, calling unload on each. Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
joewalker/gecko-dev@53af479#commitcomment-10346841 Signed-off-by: Joe Walker <[email protected]>
Remove invalid header comment. Enable working from frozen data objects. Use Function.apply rather than eval() Signed-off-by: Joe Walker <[email protected]>
Allow a command to execute another command. This means: * re-enable the deprecated updateExec function from executionContext * sidestep the problems of simultaneous executions by spinning up a new Requisition in _contextUpdateExec * add tests Signed-off-by: Joe Walker <[email protected]>
Signed-off-by: Joe Walker <[email protected]>
|
The commits after 40c8105 "Add system.destroy()" have been cherry-picked into gecko-dev (as part of joewalker/gecko-dev#11), so you can review them there. I don't think you should need to review here any more. |
joewalker/gecko-dev#11 (comment) Signed-off-by: Joe Walker <[email protected]>
This gives us formatted output rather than the default compressed output. Signed-off-by: Joe Walker <[email protected]>
This code doesn't reach mozilla-central. No need for detailed review. It's essentially a *much* shorter version of Task.spawn() from https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Task.jsm Signed-off-by: Joe Walker <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first couple of changes are to things that I'd like to get right before the remote protocol gets set in stone (or at least harder to change)
But then it's mostly about strengthening tests to make sure that the
nodeandresourcetypes work remotely.