Skip to content

Error: infinite loop calling func from another case #19

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

matthewkastor
Copy link

An error is thrown when calling a function that hasn't been defined in
the suite. If the function was defined in the same test case then it
will run. If the function was defined in another test case, it looks
like the index range is remembered but the commands are taken from the
current test case. When those commands execute successfully, infinite
recursion occurs.

It looks like functions are almost global. They should be, it would be
much more useful to define all the functions in one "case" that could be
used like a library, then call those functions in cases that come after
the library case(s) was/were loaded.

An error is thrown when calling a function that hasn't been defined in
the suite. If the function was defined in the same test case then it
will run. If the function was defined in another test case, it looks
like the index range is remembered but the commands are taken from the
current test case. When those commands execute successfully, infinite
recursion occurs.

It looks like functions are almost global. They should be, it would be
much more useful to define all the functions in one "case" that could be
used like a library, then call those functions in cases that come after
the library case(s) was/were loaded.
I forgot to turn off the option to remember base urls.
This doesn't really work yet. It looks like it works sometimes but it's
really not.
I can call functions defined in one test case, from another test case. I
can't run tests that have functions in them more than once or they'll
always fail.

When I run the script where the function is defined, it works the first
time. If I run it again then things blow up. I can run the test that
calls the function as many times as I want and it works.

I wonder if I define another function in another test, if things would
blow up when I ran the suite...
functions are defined in two different test cases. After they've run, I
can call the functions defined in them from a third test case. I can
call the functions as many times as I want to from there. Things get
weird when I'm trying to call the functions from the test case they're
defined in.
I've got better ideas about how to handle global functions. Throwing out
the selblocks global stuff I merged in and reverting to the current
master version.
Since the blockdefs and stack are reset on a per testCase basis, the
symbols need to be reset as well. If they're not, then the function
names persist between test cases but the blockdefs don't. This causes
selblocks to execute the range of indexes stored for the function, but
using the commands array from the current case, which is obviously
wrong. Resetting the symbols makes the behavior "functions aren't
global", correct and fixes the bug.
This holds a naive clone of the symbols, commands, and blockDefs for
each test suite as it is run through "compileSelBlocks". The cache is
private to the closure where selblocks is built so, it can't be tampered
with from the outside.
calling functions from other test cases should require qualifying the
function name with the test case title, that way two cases could have a
function with the same name and not collide. Think of it like two
namespaces. Currently, this doesn't execute functions in other cases, it
just uses the cache to lookup the symbol for the current case.
calling a function in the current case works as expected. Calling a
function with it's qualified name in another test case will exhibit the
same behavior as was happening before we reset the symbols for each
case.
I don't want the "function" or "endfunction" to be in the same row as
the "call".
I'd rather return clones of the commands, since we're running rows from
functions more than once. My naive clone function destroyed command
objects though.
@matthewkastor
Copy link
Author

This passes the test suite, fixes the bug, and adds the ability to call functions defined in other cases.

The way you'd call a function named "bob" in the test case titled "funkLib" from the test case "randomTests" would be:
call | funkLib.bob

Tada! XD The code didn't change much.

bugged, when parameter defaults to undefined and fn is called without
specifying said param, storedVars will not be restored on exiting fn
execution in some cases.
we have to cache the values of all storedVars that exist in the function
signature, since we're initializing them to undefined for the function
block. If we don't cache all the storedVars spec'd in the sig., then
when the call completes only the args specified in the call will be
restored while the unspecified params will be left as undefined on the
storedVars object, or leak whatever value may have been assigned to them
in the function call.
the tests now check that parameters can have default values specified,
that those values will be respected, and that parameters automatically
become function scoped variables (not just the specified args).
current command is supposed to return the current command being
executed. This has to be overridden in order to return the correct
command object.
Functions scope their defined parameters by name, all other variables
leak to the containing scope. The current behavior in master branch is
that all arguments passed in are arbitrarily scoped but, any new
variables defined inside of the function block will leak to the global
storedVariables and persist after the function returns.

This new behavior is more consistent with the model presented by
JavaScript, where defining variables without the var keyword will leak
them into the containing scope. Given that the parameters are
automatically scoped to the function, there is no need to define them
and they will not leak. This is the exact behavior exhibited by
selblocks functions now. We can implement block scoping for all
variables by cloning storedVars at the beginning of function execution,
then replacing it afterward. This is not the current behavior though,
and would more likely break existing tests that count on being able to
set global variables from within a function.
doScript now passes the second argument from the command row to
doFunction. Without passing this empty string, an error is thrown. With
this argument available, scripts could have defined parameters like
functions do... but they're the same thing really so...
This function just marks the status of the current row in the IDE. It
should follow the debug index and be called normally. I thought it was
doing something else earlier.
Store adds any var you want to the storedVariables object. The only
properties that will be reverted / deleted after function calls are the
explicitly named parameters of the function.
@matthewkastor
Copy link
Author

Now supports defining parameters for functions, with user specified default values. If no default value is given they'll initialize to undefined.

Define the function
function | someAmazingFn | param = "awesomeDefault", otherParam

call the function and param will have the value "awesomeDefault", otherParam will exist in storedVars but will be undefined.
call | someAmazingFn | |

Only the defined parameters are copied / restored (block scoped), which is different than before. It used to be that any given arguments were the only block scoped variables. The behavior in this branch is like JavaScript. The store command just makes "global" variables all the time and I don't know that I'd want to change that. Maybe a different command like let for block scoped variables would be a good idea. That's something else entirely though. This is ready to use.

Adding a new selenium command "SetLocal" to set variables that will be
discarded when the block completes.
Local storage is implemented through the ContextManager. Currently it
does not enter and exit contexts though, so nothing would appear to
change... unless I've got a typo in the code or forgot to wire
blockVars, storedVars, and _oStoredVars in... Just checking in the new
additions mostly.
I've implemented block scope for function variables. storeLocal stores
local to the block, storeGlobal stores to the global variables. Both
local and global variables can be expanded when using the ${} notation.
The original store commands, or anything that tries to write directly to
storedVars will be subject to the new block scope mechanism.

To start a new block scope just write "contextManager.enter()", to
return to the parent block scope just write "contextManager.exit()" They
should be able to nest for as long and large as a javascript prototype
chain is allowed to be.
for some stupid reason you can't change the store methods on the
selenium prototype. Probably because they're cut and pasted over and
over and over all over the IDE codebase and reloaded from who knows
where, who knows when...
There's no way to make "store" store variables in global scope and
implement block scoping at the same time. They're reloading
selenium-runner after loading the plugins and refreshing the default
definition for doStore before every command is executed. All we'd need
to do is point "doStore" at the new "storedVarsGlobal" and things
wouldn't break but.. yeah.
Entering and exiting context in doCall was too early and too late. Moved
context change into doFunction and doEndFunction.
selbench stores "emit" in the storedVars. storedVars is actually block
scoped now so the emits are wiped out when the function returns. I'm
returning the values that were emitted before and asserting their
correctness immediately.
The built in storeEval makes variables that are local to the block
scope. This adds the ability to do the same but in global scope.

I had exited the context too early in returns, so variable expansion in
return values was broken for a minute.
The iterator value "w" is evaluated and stored inside the "with" block.
The iterator emits the right values but after the block is completed,
emitting "end ${w}" gets the value of w in the scope above the "with"
block, which hasn't been changed.

The variable is automatically shadowed in local scope, so storing the
iterator value globally doesn't work because the updated value isn't
visible locally. I don't know how that works exactly, it's something
with the way arguments are being parsed.
the value for i will be the same at the start and end because it's
incremented in block scope to the for loops.
this test is really convoluted. It looks like "finally" is supposed to
execute even after a return statement. That finally throws, so nothing
is being returned.
@matthewkastor
Copy link
Author

Block scoping works everywhere now. the builtin store commands write to the local scope and there's nothing I can do to change that. It used to just make global variables so this will break scripts. An easy way to fix a script broken by this is to do a search for <td>store</td> and replace it with <td>storeGlobal</td>.

The increment variable used in while, for, etc. loops will not appear to change if you check it's value before and after the loop executes. This is because it's manipulated inside the block's scope and shadowed locally. It ends up shadowed locally because of the call to evalWithStoredVars on the conditional. When the block exits, the scope is lost, and you'll see the value of the incrementor from the parent scope as it was before entering the block.

If you try to use global variables in your loop conditions then it won't work. Since the loop condition's variables all end up being copied into local scope, you won't be able to see the values of the globals using the ${} notation. You'd need to access them through js using storedVarsGlobal.

While inside a block you can set global variables and read their global values, just as long as you don't create a local variable that shadows them.

There are some changes to the existing tests to reflect the new behavior. There is a new test suite with all the new tests I've written for the new features introduced, it's in GlobalFunctionsTestSuite.html You'll need updates to SelBench in order for the test suite to pass, since emit and others rely on global variables. refactoror/SelBench#8

This is ready again and passes all the tests.

I was setting and verifying at the same time, which is wrong.
The tests explain it better than I can. Basically, without this you only
have the choice of shadowing variables in the child scope, or setting
globals all the time to pass data around. storeAt allows you to name a
variable defined in any preceeding scope and bubble the value to set on
it. The changed value will be available after you exit the current
scope, so you can do things like "while" and "for" loops without
bouncing return values off the global scope. . . Just storeAt whatever
receiving variable you have set up in the parent and it's like magic!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant