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

fn:base-uri should not raise XPDY0002 when the context item is empty #3498

Merged
merged 1 commit into from
Aug 12, 2020

Conversation

adamretter
Copy link
Member

Closes #3497

@adamretter adamretter added the bug issue confirmed as bug label Aug 5, 2020
@adamretter adamretter added this to the eXist-5.2.1 milestone Aug 5, 2020
@adamretter adamretter requested a review from a team August 5, 2020 10:37
Copy link
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

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

Nice!
I would personally split the test case resolveBaseURIErrorCases in to two or even three separate tests.
One for ()/fn:base-uri() (one for ()/fn:base-uri(.)) and one for fn:base-uri(.) (fn:base-uri() seems to be untested).
Especially, since the first two are not error cases.
Generally, because a test should assert one single thing. But that would mean three separate tests.
On a side note: why is the test implemented in Java instead of adding those tests to the XQSuite?

@adamretter
Copy link
Member Author

adamretter commented Aug 5, 2020

@line-o The existing tests for fn:base-uri were already in Java, so I continued that way for continuity. Also I still don't trust XQSuite entirely.

Many of the Java tests assert behaviour rather than a single thing, so I don't think my approach is wrong necessarily.

@dizzzz
Copy link
Member

dizzzz commented Aug 6, 2020

Ok for me

@joewiz
Copy link
Member

joewiz commented Aug 12, 2020

@line-o I see your agenda item from Monday's call:

I would suggest to stick to XQSuite tests wherever possible because they are more accessible to developers unfamiliar with java.

I don't see any notes detailing whether this was discussed, but it seems to be a good suggestion going forward - which I would articulate as: use xqsuite as a default, with the caveats that of course there may be compelling reasons to do otherwise, such as continuity/grouping with other similar tests (so as not to split substantively similar tests across different locations), or to test functionality that xqsuite cannot handle.

Since we already have working tests here that meet this principle and have secured approval, I will merge this PR.

@joewiz joewiz merged commit abd1c4f into eXist-db:develop Aug 12, 2020
@adamretter
Copy link
Member Author

@joewiz I actually really strongly disagree. As an eXist-db developer, XQSuite tests are horrible to debug, the ones written from Java are much easier to work with. Also I don't trust that XQSuite doesn't still have some false-positives etc. If our XQuery implementation were more spec compliant, I might begin to trust XQSuite further.

@joewiz
Copy link
Member

joewiz commented Aug 12, 2020

@adamretter Re: test suites, let's discuss this at a community call. Since there were no notes, I presume it wasn't discussed, so the agenda item will carry forward to next week's call.

@adamretter adamretter deleted the hotfix/base-uri-context-item branch September 14, 2020 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issue confirmed as bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ()/fn:base-uri() incorrectly raises XPDY0002
4 participants