Issue 1793

[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 :
• background executed for each row even for dynamic scenario outline #1793
• org.graalvm.polyglot.PolyglotException: TypeError, failed due to: Message not supported #1766
• 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 apologies for the previous PR. Fixes #1793 and #1766.

The fix for 1793 was unrelated and I added a workaround. You might have a better idea on how to solve it:

• when the headers of an HTTP request are a function and we're executing in Scenario Outline dynamic (i.e. background executed once and there's reusability across multi thread) the multithreading error is caused when multiple threads try to access and execute the same function on ScenarioEngine:568
• a possible fix might be on the constructor of Config to create a copy of the functions, if any, in this headers. I have not tried but there's a chance that functions on cookies and on the afterScenario and afterFeature also cause the same error and so we might not want to release this code just yet
• I didn't want to touch the Config constructor just yet to leave you some time to think

Still pending fixing the unit test on FeatureRuntimeTest.testJsRead but I need to step away from my laptop - will try to get to it next weekend.

[joelpramos]

Issues are fixed... but still with the workaround using the synchronize on the http headers. My only other thought to avoid the synchronize is to try to use the ProxyInstantiable from oracle/graal#631 (comment) - in JsValue we don't cast the js functions Source into that proxy - caught my attention as the odd difference.

[ptrthomas]

@joelpramos LGTM ! thanks

[ptrthomas]

Issues are fixed... but still with the workaround using the synchronize on the http headers. My only other thought to avoid the synchronize is to try to use the ProxyInstantiable from oracle/graal#631 (comment) - in JsValue we don't cast the js functions Source into that proxy - caught my attention as the odd difference.

@joelpramos I think you are right, so in the graal refactor I am doing now #2009, this approach seems to be working well. created a new branch and working through it - so here's the relevant part of the commit. the synchronize solves the multi-thread problem even in JS functions shared across contexts: 90a59bd#diff-3cf165c3dfa33cc95a64822b2d0f910c549688cf971a0aaa75a2b1d63370ff27R397-R404

I think in this PR you worked on making the http builder be "sharable" across called features. my opinion has always been that when you call a feature, the builder "resets". the only place where the builder can be "half baked" is in the Background. r

efer this issue: #2054 and this one for examples of the confusion: #1990

so I plan to make this change, let me know if you have concerns.

I know we currently have some inconsistent behavior for "dynamic scenario outlines" - so what I plan to do is make everything consistent - Background will run before every Scenario even if it is a dynamic scenario outline.

which means there will be a question - how do we "setup" a JSON array that can be injected into a "dynamic scenario outline" ? - and I propose to make this the approach, and this is likely to be a breaking change: #1905

https://github.com/karatelabs/karate/pull/1828/files#diff-17804956652011cd7bb614a6c9e7a9bfaff093f5e811881a0224355ec98aea30R42-R54

[joelpramos]

I think fundamentally I disagree that background should run for each iteration/scenario because of my interpretation of the keyword.

Regardless I do agree that inconsistency does no one no good and appears that call once, that other ticket for the JSON data, and maybe some Js and variable replacement on the Examples table covers most scenarios. If we’re making a breaking change might as well be with the Graal refactor which might still bring some surprises.

for purposes of discussion - why not change the behaviour of the Background on normal Scenario / Scenario Outline? Do you see background primarily as a section to reduce duplicated instructions for a scenario/test?

[ptrthomas]

@joelpramos my interpretation of Background has always been that it is the equivalent of the @Before in JUnit. this is also how it behaves in Cucumber. and I believe this is what most developers expect. that it is for setting up data, local to a feature - but common to multiple test-scenarios

I consider callonce as an exception to the rule.

[joelpramos]

I see. Going to junit I think I'd say @BeforeClass. If that's how Cucumber works it's only natural to keep Karate similar as that's where you'll find most of the target persona.

I'll take a look into the related tickets and keep an eye on your refactoring branch based on my time.