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

Add more tests to ensure proper handling of undefined #131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cjbarth
Copy link

@cjbarth cjbarth commented Sep 19, 2023

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.

@JLRishe
Copy link
Collaborator

JLRishe commented Dec 16, 2023

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.

@cjbarth
Copy link
Author

cjbarth commented Dec 27, 2023

@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.

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

Successfully merging this pull request may close these issues.

2 participants