Adding unit tests to test and reproduce several scenarios with readin…

[joelpramos]

Description

Thanks for contributing this Pull Request. Make sure that you submit this Pull Request against the develop branch of this repository, add a brief description, and tag the relevant issue(s) and PR(s) below.

• Relevant Issues : 1.0 release thread #1373 (comment)
• Relevant PRs : (optional)
• Type of change :
  • New feature
  • Bug fix for existing feature
  • Code quality improvement
  • Addition or Improvement of tests
  • Addition or Improvement of documentation

[joelpramos]

@ptrthomas minimal scenario to replicate #1373 (comment) is the js-read-4.feature :

js-read-4.feature

Feature:

Background:

Scenario:
    * def params = { 'foo': 'bar' }
    * call read('js-read-called-2.feature') params

js-read-called-2.feature

Feature:

Background:
    * call read('../utils-reuse-common.feature')

Scenario:
    * print 'arg: ' + __arg
    * match __arg == "#present"
    * match __arg == "#notnull"

Issue happens when the called feature calls and the called feature calls another feature in background adding it's variables.

[joelpramos]

After checking this in I realized I could try this:

js-read-called-2.feature

Feature:

Background:
    * def called = call read('../utils-reuse-common.feature')

Scenario:
    * print 'arg: ' + __arg
    * match __arg == "#present"
    * match __arg == "#notnull"

And it works. The difference is that on the the called feature we are assigning the result of another called feature to a variable. So it's probably the piece of code that "adds" the content/variables of the called feature in the background that is somehow overriding/removing the __arg .

I haven't tried other keywords yet like __row , __num for Outlines

btw the print statement returns this:
13:47:26.320 [main] INFO com.intuit.karate - [print] arg: [object Object]

It's just the print statement. Added a match to the PR and the content of __arg can be easily used.

[ptrthomas]

@joelpramos I'm lost. any way to simplify - just focus on 1 case at a time instead of so many tests. also I see a build failure. will look at this later

[ptrthomas]

@joelpramos no worries. I just may have realized what the problem could me. just making one commit before logging off - hope that solves !

[joelpramos]

I got it - was refactoring now so that the magicVariables stay only within the scope of the ScenarioRuntime (ie don't add to the vars Map) but are added to the JS Engine. Let me know if that's what you're working towards.

++ and keeping any other variables outside of that magicVariables map so they are propagated with shared scope

[joelpramos]

@ptrthomas made the changes for the magicVariables being available only within the Scenario or Scenario Outline being executed - ie if a scenario is called that scenario won't have changes to __arg (for e.g.) even if in shared score.

When reviewing let me know if this would be an intended design that aligns with your thoughts. Also check out
ScenarioRuntimeTest.testCallFromJs()
ScenarioRuntimeTest.testCallKarateFeature()

Especially the later. That was an unintended consequence in which those variables are now not getting returned when assigning the result of a called feature (diff in the PR should be obvious). I can give it some thought into how to work around it / make it work as before if you think it's important (maybe adding in the scenario result when the execution of the scenario is over could be an option but my concern is it's complex as we will "pollute" the variables of the caller).

[joelpramos]

forgot to remove the comment from the check in

[joelpramos]

need to add the other two vars in the comment for documentation purposes

[ptrthomas]

@joelpramos ok so I made one change which puts the background in the engine variables. which hopefully solves some of the problems you outlined - but I'm finding it hard to understand some of it. btw I don't handle large diffs well. and this is one of the most complex areas of karate. some thoughts below

• I've never encouraged calling other features. in my mind it should only be used one-level and the called feature should IMO never do any outline business especially dynamic
• so if some of the variables get over-written, especially in "shared scope" scenarios I'm fine with it. leave it to user to handle
• I do not want to add complexity to setVariables() which can be called during a scenario execution. any complexity should be in the constructor or "once only" init()
• note that when you add callonce or callSingle() into the mix - it may open up a whole bunch of edge cases on top of these (background, dynamic, outline, call, shared scope, passed arguments)

so I propose

a) see if my fix solves 80% of the problems
b) please create ONE test to show the next most high-priority issue in your opinion and start a new PR if needed and we can work through one at a time

[ptrthomas]

@joelpramos ok I think I have a solution for all the problems, explained / details in this commit: 426e9c0

so maybe all we need is some extra tests that you see fit !

[joelpramos]

@ptrthomas Yeah those changes would work (although I haven't run the unit tests yet).

I'd suggest one more thing - see the changes I did at ScenarioRuntime:initMagicVariables().

In that method we are adding to the map the variables from the caller feature and the variables from the Examples table. With the Map stored on that variable we will iterate add set those variables as hidden variables (your code change in the Scenario Engine).

I haven't tried your PR yet with the tests but as a principle I'd suggest we initialize only those special variables in that method and do a setVariables() for both caller and examples table variables to the ScenarioRuntime vars property in the Engine. See here: ScenarioEngine.java:init() in my PR.

Proposed additional snippet in the init() method below:

        runtime.magicVariables.forEach((k, v) -> setHiddenVariable(k, v)); // your changes

        // proposed extra code
        if(this.runtime.scenario.isOutlineExample()) {
            Map<String, Object> exampleData = runtime.scenario.getExampleData();
            this.setVariables(exampleData);
        }
        if (this.runtime.caller.arg != null && this.runtime.caller.arg.isMap()) {
            this.setVariables(this.runtime.caller.arg.getValue());
        }

My gut feeling is that your PR works completely because there's a line here that sets all the magicVariables from the caller and so it doesn't really make a difference but as a design principle I'd keep that function initMagicVariables() only with the responsibility of keeping those indeed magical variables.

The responsibility of setting variables as part of the shared context or not is already delegated by the caller in which the vars map is either passed as reference or a deep copy (which come to think of it passing those variables in the initMagicVariables() might actually give "write" access to called features in when context is not shared).

[joelpramos]

As per unit tests - I'm pleased with the ones on this PR, once we finish the work in the develop branch I'll close this one and open a new one with just the tests. I'll do a review to confirm whether I've covered all the scenarios with callonce as well (probably not as you can see there were a lot of different cases I could think of :) )

[ptrthomas]

@joelpramos

might actually give "write" access to called features in when context is not shared

the "hidden" variable approach solves this problem nicely they will not be "mutated" by called features to any depth. which also may be the reason why your other concerns may not be valid. do have a closer look and see what you think, I felt it is reasonably elegant.

I think we will have cases where users make a call (just after landing from another call) and expect variables to be in scope (even though I hate this) hence the need for copying parent variables over. lets see if you can find any edge cases via your tests

[joelpramos]

Closed as per discussion. See unit tests at this PR #1438