xpath icon indicating copy to clipboard operation
xpath copied to clipboard

Add more tests to ensure proper handling of undefined

Open cjbarth opened this issue 2 years ago • 2 comments

When using this code over at node-saml, I found that we were sending undefined or null on occasion. I've added some tests, but changed no behavior, to validate what was actually happening internally. I then updated the TypeScript types to match the tests.

This PR focuses on minimal changes for correctness involving these new tests, so I didn't address any of the broader feedback on types.

cjbarth avatar Sep 19 '23 10:09 cjbarth

I'm sorry for leaving this project unattended for so long. The flurry of incoming requests in July got overwhelming and drew time away from things that I needed to be spending time on, and I developed a mental block against spending any time on this.

Thank you for proposing these changes.

Today, I've added some additional error handling to throw somewhat helpful errors when:

  • The xpath expression is unspecified
  • The xpath expression is not a string
  • The context node is unspecified in a situation where one is needed (there are situations where one is not needed)
  • The context node is specified but not node-like

As of these changes, the first test you've proposed is no longer relevant because an unspecified expression is now an error condition and I have added a test for that. This also makes most of the .d.ts changes in this PR moot, though I think the renaming of SelectedValue to ScalarValue is good.

Your second and third tests, I would say, "incorporate too many variables" because in both of them, you are passing an undefined expression at the same time that you are passing an undefined document or resolver (maybe this was unintentional?).

With regard to the second test, hopefully my new error handling and two new tests address this, in a slightly more thorough manner.

For the third test, I think it's painting a bit of an incomplete picture. Not passing a resolver can result in an empty result set, but only under certain circumstances. And as I say above, the situation in your test is muddied by the lack of an XPath expression. But if you can identify a situation where omitting the resolver results in an outcome that one would not reasonably expect, I may be able to find a way to improve that situation.

Thanks again.

JLRishe avatar Dec 16 '23 14:12 JLRishe

@JLRishe , I can make another PR to change this to be just the relevant parts. If you are open to #132 , I'd like to land that first after a rebase, of course.

cjbarth avatar Dec 27 '23 19:12 cjbarth