Fix issue 393 (x:call-scenarios template references 'this' element) #562
Conversation
# Conflicts: # src/compiler/generate-common-tests.xsl # src/compiler/generate-xspec-tests.xsl # test/generate-xspec-tests.xspec
# Conflicts: # .travis.yml # appveyor.yml
# Conflicts: # .travis.yml # appveyor.yml
# Conflicts: # test/end-to-end/cases/expected/query/xspec-153-result.html
# Conflicts: # test/generate-tests-utils.xspec
# Conflicts: # test/end-to-end/cases/expected/query/xspec-67-result.html # test/end-to-end/cases/expected/query/xspec-three-dots-result.html # test/end-to-end/cases/xspec-67.xspec # test/end-to-end/cases/xspec-three-dots.xspec # test/generate-tests-utils.xspec # test/generate-x-utils.xspec
# Conflicts: # test/generate-tests-utils.xspec
# Conflicts: # test/win-bats/collection.xml # test/xspec.bats
# Conflicts: # test/end-to-end/cases/expected/query/xspec-three-dots-result.html
…01-22 # Conflicts: # .travis.yml # appveyor.yml
# Conflicts: # src/reporter/format-xspec-report.xsl # test/end-to-end/cases/expected/stylesheet/xspec-serialize-result.html # test/end-to-end/cases/expected/stylesheet/xspec-three-dots-result.html
|
@galtm |
I'll try to review it this week. |
| @@ -168,7 +188,7 @@ | |||
| corresponding call instruction at some point). | |||
| --> | |||
| <xsl:template name="x:call-scenarios"> | |||
| <xsl:param name="pending" select="this//@focus" tunnel="yes" as="node()?"/> | |||
| <xsl:param name="pending" select="(.//@focus)[1]" tunnel="yes" as="node()?"/> | |||
There was a problem hiding this comment.
Here is what I think I'm seeing in the XSLT case. I haven't looked at the XQuery case as much.
When the name="x:call-scenarios" template operates on x:description, the default value is assigned to the $pending tunnel parameter. However, the value does not matter because the generated name="t:main" template needs to call the generated templates corresponding to all the top-level scenarios, whether they are pending or not. (Pending status affects deeper-level parts of the generated code.)
When the name="x:call-scenarios" template operates on x:scenario, the default value of the $pending tunnel parameter does not get assigned because a value came from the calling name="x:output-scenario" template. Before that, the $pending value came from the calling match="x:scenario" mode="x:compile" template's $new-pending variable, which reflects whether the scenario being operated on should be treated as pending.
Please correct anything that sounds inaccurate. I want to understand the compiler code and it's complicated.
I agree with you that the default does not matter in the current code path. As for possible future changes, I'm not sure whether select="(.//@focus)[1]" or select="()" is more suitable; maybe whoever makes the future changes will need to think through the initialization and propagation of $pending based on what those changes are trying to do. I do think that what you have makes more sense than a reference to a this element that does not exist.
Is there value in adding a comment like, "Default value of pending does not affect compiler output but is here if needed in the future"? That could save someone time trying to figure out what you now know. If you think it's extraneous, it's fine not to add it.
There was a problem hiding this comment.
Thanks for the invaluable review. Adding a comment is a very nice idea! I added it to this pull request: a98f70d
There was a problem hiding this comment.
You're very welcome! Thanks for adding the comment.
# Conflicts: # test/end-to-end/cases/expected/query/xspec-report-junit.xml # test/end-to-end/cases/expected/query/xspec-report-result.html # test/end-to-end/cases/expected/query/xspec-report-result.xml # test/end-to-end/cases/expected/stylesheet/xspec-report-junit.xml # test/end-to-end/cases/expected/stylesheet/xspec-report-result.html # test/end-to-end/cases/expected/stylesheet/xspec-report-result.xml # test/end-to-end/cases/xspec-report.xspec
This pull request derives from #561. So needs to be handled after that.
Fixes #393
Looking back the change history, the following changes are worth noting:
The responsibility of setting the default value of the
$pendingtunnel parameter was distributed to thex:call-scenariostemplate and thex:compile-scenariostemplate: 82c8d10The default
$pendingvalue in thex:call-scenariostemplate was mis-edited while rewriting: 36da391The default
$pendingvalue was updated only in thex:compile-scenariostemplate: 3366f94This pull request fixes the mis-edit (happened in 36da391) and follows the update (happened in 3366f94).
I can't provide a specific test for this fix, because the default
$pendingvalue in thex:call-scenariostemplate does not seem to take effect in the current code path. It may take effect in the future, so I propose this fix now.The overall tests are in
xspec-focus-1.xspec,xspec-focus-2.xspecandxspec-pending.xspecand slightly improved by #561.