Skip to content
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

try-catch support: x:scenario/attribute(catch) #599

Merged
merged 19 commits into from
Sep 25, 2019
Merged

Conversation

AirQuick
Copy link
Member

@AirQuick AirQuick commented Sep 5, 2019

Closes #500

This pull request introduces x:scenario/@catch.

With <x:scenario catch="yes">, its descendant-or-self scenarios catch dynamic errors when invoking the tested module. The standard error variables including $err:code and $err:description are stored in $x:result as a map. If the tested module didn't throw an error, $x:result contains its return items intact as usual.

Because the result is a map, @catch requires XSLT 3.0 or XQuery 3.1 (only when you set catch="yes").

For Schematron, @catch will work just like XSLT, although it won't make sense.

Commits

  • 6741118
    Defines boolean.datatype in the schema, aligning with XSLT boolean.

  • 117a125
    Sets up a utility function to convert boolean.datatype to xs:boolean.

  • 4119b78
    Defines @catch in the schema.

  • f7a73a8
    Implements @catch.
    Due to indentation, the diff appears larger than the actual change. The actual change is simple; it just puts the existing function call, xsl:call-template and xsl:apply-templates into try-catch when ancestor-or-self::*[@catch][1]/@catch is true.

  • 1f0bc3e
    Test for XSLT and XQuery. This also demonstrates how you would use @catch.
    Schematron is not tested at all, because @catch doesn't make sense on Schematron.

  • 5b0df56
    Test for XSLT and XQuery to make sure that errors are not always caught.

@AirQuick
Copy link
Member Author

AirQuick commented Sep 5, 2019

@galtm
Can you review this in your spare time? (we're in no haste)

@cmarchand , @Nico-Amplexor , @cmsmcq
Please let us know your thought on this proposal, if you're still interested.

@galtm
Copy link
Member

galtm commented Sep 5, 2019

Can you review this in your spare time? (we're in no haste)

Yes. This looks interesting.

@Nico-Amplexor
Copy link

Please let us know your thought on this proposal, if you're still interested.
@AirQuick, it seems a pretty good solution ;)

@AirQuick AirQuick added this to the v1.5.0 milestone Sep 8, 2019
Copy link
Member

@galtm galtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read through almost all the file changes, and they look great. So far, here are my questions and an observation:

  1. Setting @catch on a shared scenario containing x:call or x:context appears not to have any effect on the scenario where the corresponding x:like element occurs. Is that what a test author would expect? If you don't think @catch on a shared scenario should be considered, then would it be useful to give test authors a hint via the schema (either in RelaxNG or Schematron)?

  2. What is the purpose of storing the line and column numbers in $x:result?err, given that they refer to the compiled code rather than the SUT? Are these numbers for people debugging the XSpec infrastructure itself and not intended to be useful to XSpec end users?

  3. I like how the tests under test/catch/ are setting boundaries for what should be caught. Have you considered adding tests for these other situations where errors should not be caught? (a) A mistake in construction of the XSpec scenario that causes a compiler error, such as x:expect without x:call or x:context; (b) A static error in the compiled test, such as calling a function that does not exist or calling a function like normalize-space on a sequence of multiple strings.

  4. If the error you catch is not the one you expected, you'd probably like the HTML report to help you find the discrepancy. If your assertions are all on the subordinate fields within $x:result?err, the report looks great. If you do test="$x:result?err", the HTML report from the XSLT test shows helpful information while the HTML report from the XQuery test shows only <pseudo-other />. You might even get two pseudo-other elements colored green on both sides of the report, e.g., if you do

<x:expect label="How does nonmatching err appear in report?" test="$x:result?err"
  select="map{'something':100}"/>

Does it make more sense to improve this formatting in the XQuery report or merely document a best practice of checking the subordinate fields?

  1. Observation: I noticed you updated code for x:apply even though that is not fully implemented yet and the updates can't really be checked yet. That was a background question I had about issue Namespace declarations need to be on x:description element (not a descendant), in some circumstances #598. As I continue working on that, I'll try to insert code in the x:apply case to hopefully make it easier for someone to finish the x:apply implementation someday.

@galtm
Copy link
Member

galtm commented Sep 13, 2019

2. line and column numbers

Please ignore my comment 2. Those numbers do refer to the SUT, as a test author would expect.

@AirQuick
Copy link
Member Author

  1. Setting @catch on a shared scenario containing x:call or x:context

The expected behavior of the shared scenario is somewhat unclear. (#477)
This is one of the areas I'd like to defer to "Details are subject to change. Any feedback is welcome."

I pushed two commits:

  • 6e10c75 to see how @pending behaves.
  • 78677d5 to see how @catch behaves.

Looks like @pending and @catch behave in the same way. (No effect when pulled by x:like.)
So, at the moment, I would consider @catch is working as implicitly expected.

I commented #477 (comment) to resolve this unclearness there.

@AirQuick
Copy link
Member Author

  1. ... the HTML report from the XSLT test shows helpful information while the HTML report from the XQuery test shows only <pseudo-other />.

This is one of few things where XQuery support is behind XSLT.

(: TODO: function(*) including array(*) and map(*) :)

When I submitted the XSLT part in #546, I had no idea how to implement it in XQuery. (I personally do not use XQuery at all and know little about it.)

@AirQuick
Copy link
Member Author

  1. ... (a) A mistake in construction of the XSpec scenario that causes a compiler error, such as x:expect without x:call or x:context; (b) A static error in the compiled test, such as calling a function that does not exist or calling a function like normalize-space on a sequence of multiple strings.

Good idea. Pushed (a) a50b401 and (b) 91dddb0.

@AirQuick
Copy link
Member Author

  1. Observation: I noticed you updated code for x:apply even though that is not fully implemented yet and the updates can't really be checked yet.

Pushed a7e9a34 to clarify the current status of the code path.

@galtm
Copy link
Member

galtm commented Sep 13, 2019

@AirQuick , thanks for your responses to my questions and observation. I finished reviewing the files and your newer commits, and I don't have anything else to say. I'll be happy to look again if you think of any further changes or if there are other comments on this thread.

@galtm galtm self-requested a review September 13, 2019 20:16
Copy link
Member

@galtm galtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've addressed the questions I asked earlier.

# Conflicts:
#	src/common/xspec-utils.xsl
#	test/xspec-utils_stylesheet.xspec
@AirQuick
Copy link
Member Author

Thanks @galtm for the precious review. I'll merge this some time next week.

@AirQuick
Copy link
Member Author

Added Testing Dynamic Errors to Wiki.

@AirQuick AirQuick merged commit 71b6e44 into xspec:master Sep 25, 2019
@AirQuick AirQuick deleted the catch branch September 25, 2019 22:58
@AirQuick
Copy link
Member Author

Now merged. Thanks @galtm for the review.
Also thanks @cmarchand , @Nico-Amplexor , @cmsmcq, for the proposal and the suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[XSLT 3.0 Support] Managing errors
3 participants