Skip to content

Improve test fromRdf#t0027.#669

Merged
BigBlueHat merged 3 commits into
mainfrom
update-fromRdf-t0027
Jun 10, 2026
Merged

Improve test fromRdf#t0027.#669
BigBlueHat merged 3 commits into
mainfrom
update-fromRdf-t0027

Conversation

@davidlehn

Copy link
Copy Markdown
Contributor
  • Improving the test coverage for use useNativeTypes algorithm in 8.5.2 2.4.
  • Remove the "True" and "False" boolean tests that can cause confusion that those words are special values.
  • Add tests using a value of "notnative" typed to boolean, integer, and double. These test that the type will be preserved when a native conversion does not happen.

I was fixing jsonld.js for fromRdf#t0027 and thought the tests could use some work to increase covearge.

https://github.com/w3c/json-ld-api/blob/main/tests/fromRdf/0027-in.nq
https://github.com/w3c/json-ld-api/blob/main/tests/fromRdf/0027-out.jsonld

I initially mistakenly tried to add specific support for the "True"/"False" tests:

<http://example.com/boolean-object> <http://example.com/example> "True"^^<http://www.w3.org/2001/XMLSchema#boolean> .
<http://example.com/boolean-object> <http://example.com/example> "False"^^<http://www.w3.org/2001/XMLSchema#boolean> .

But those are, I think, just testing that any not-native value is passed through even though it may not match semantics of the type. I think some other value text should be used to avoid confusion. For consistency, it probably makes sense to have similar tests for integers and doubles. jsonld.js was dropping the type for non-integers typed as integers.

There might be other edge cases that should be tested here too. This does seem like an incremental improvement.

Possible relevant references:
https://w3c.github.io/json-ld-api/#algorithm-16
#555
#656
#619
#625
#662

- Improving the test coverage for use useNativeTypes algorithm in 8.5.2
  2.4.
- Remove the "True" and "False" boolean tests that can cause confusion
  that those words are special values.
- Add tests using a value of "notnative" typed to boolean, integer, and
  double. These test that the type will be preserved when a native
  conversion does not happen.
@filip26

filip26 commented Oct 29, 2025

Copy link
Copy Markdown
Contributor

I see it now, the original test suite only considered "true" and "false" as valid values for xsd:boolean, but since xsd:boolean also accepts "0" and "1", the test was later updated accordingly.

I would keep the "True" and "False" tests, as they ensure that no implementation is performing a case-insensitive comparison like:

"true".equalsIgnoreCase(object)

which could incorrectly coerce capitalized "True" or "False" into the native boolean type. These values are not valid according to the xsd:boolean definition and therefore should remain as-is, without conversion.

Restoring True and False tests to check only strict lexical values are
converted to native values.
@davidlehn

Copy link
Copy Markdown
Contributor Author

I would keep the "True" and "False" tests, as they ensure that no implementation is performing a case-insensitive comparison like: [...]

Ok. More tests are always good. Hopefully others don't get confused like I did and try to special case those values.

@niklasl niklasl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

Note though, that in RDF 1.2, the requirement in RDF 1.1 (3.3, bullet point 2 b) to produce graphs containing ill-typed values is relaxed from a MUST to a SHOULD in the upcoming RDF 1.2 (3.4.2, bullet point 3.2). So an RDF 1.2 processor might drop those True|False|notnative tokens since they aren't in their respective value spaces. (But they shouldn't...)

@pchampin pchampin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NB: my proposed change is just a suggestion, to respond to @davidlehn 's concern.
But I'm happy with the PR as it is.

Comment thread tests/fromRdf/0027-in.nq

@TallTed TallTed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

With PA's comment

Co-authored-by: Pierre-Antoine Champin <pierre-antoine@w3.org>
@BigBlueHat BigBlueHat merged commit 1cc0752 into main Jun 10, 2026
2 of 4 checks passed
@BigBlueHat BigBlueHat deleted the update-fromRdf-t0027 branch June 10, 2026 16:27
@w3cbot

w3cbot commented Jun 10, 2026

Copy link
Copy Markdown

This was discussed during the #json-ld meeting on 10 June 2026.

View the transcript

Pull Request 669 Improve test fromRdf#t0027. (by davidlehn) [needs discussion]

bigbluehat: approved by two people

anatoly-scherbakov: could we setup an automerge after a given number of approval?

bigbluehat: not sure about that; there are different kinds of approval

pchampin: the reason I put the "needs discussion" label is that it was approved a long time ago, no reason to let it linger
… whether or not my suggested change is included

anatoly-scherbakov: the reason I suggest auto-merge is that the probability to merge is high if several people approve and no one objects
… also, reverting a PR is cheap; we could be faster with auto-merge and rare revert, than we are today

bigbluehat: a reason we are slow is that PRs have been stacking up for a long time, which also takes us time to "restore context"

<Zakim> TallTed, you wanted to speak antiautomerge

TallTed: I'm usually on the side of letting automation help the human
… however, auto-merge because of 2 approvals does not lead to the desired effect, in my experience
… it is too easy for people to auto-approve; and reverting can also be a nightmare

pchampin: +1 to what TallTed and bigbluehat have said
… there are some things about the current situation which makes things slow
… but I think the discussions are helpful, especially as we get back into doing the work in this phase
… I was happy to have my PRs discussed
… but in interest of time, we could also give more leeway to editors to merge things more quickly
… likely via some agreed upon rules from the group
… something the Linked Web Storage group does is convert comments into issues
… which prevents blocking PRs on commentary
… so they push comments into issues to keep things in the queue

bigbluehat: I would like to see us do this more often

<TallTed> converting PR comments to new issues is a GREAT feature I've been using for some months if not years

dlehn: we just merged a comment in an n-quads file, which previously had no comment.
… Is that a problem?
… I'm not sure the custom n-quads parser we are using supports them...

bigbluehat: I think we will figure out :)
… please, if it does break, either fix it or let us know


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

Projects

Development

Successfully merging this pull request may close these issues.

9 participants