Skip to content

some more tests for the "duration" format #402

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

some more tests for the "duration" format #402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions tests/draft2019-09/format.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,5 +610,41 @@
"valid": true
}
]
},
{
"description": "validation of durations",
"schema": { "format": "duration" },
"tests": [
{
"description": "ignores integers",
"data": 12,
"valid": true
},
{
"description": "ignores floats",
"data": 13.7,
"valid": true
},
{
"description": "ignores objects",
"data": {},
"valid": true
},
{
"description": "ignores arrays",
"data": [],
"valid": true
},
{
"description": "ignores booleans",
"data": false,
"valid": true
},
{
"description": "ignores null",
"data": null,
"valid": true
}
]
}
]
102 changes: 102 additions & 0 deletions tests/draft2019-09/optional/format/duration.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
{
"description": "validation of duration strings",
"schema": {"format": "duration"},
"comment": "see https://en.wikipedia.org/wiki/ISO_8601#Durations for examples and clarifications",
"tests": [
{
"description": "a valid duration string",
Expand All @@ -12,6 +13,107 @@
"description": "an invalid duration string",
"data": "PT1D",
"valid": false
},
{
"description": "no elements present",
"data": "P",
"valid": false
},
{
"description": "no time elements present",
"data": "P1YT",
"valid": false
},
{
"description": "no date or time elements present",
"data": "PT",
"valid": false
},
{
"description": "elements out of order",
"data": "P2D1Y",
"valid": false
},
{
"description": "missing time separator",
"data": "P1D2H",
"valid": false
},
{
"description": "time element in the date position",
"data": "P2S",
"valid": false
},
{
"description": "four years duration",
"data": "P4Y",
"valid": true
},
{
"description": "zero time, in seconds",
"data": "PT0S",
"valid": true
},
{
"description": "zero time, in days",
"data": "P0D",
"valid": true
},
{
"description": "one month duration",
"data": "P1M",
"valid": true
},
{
"description": "one minute duration",
"data": "PT1M",
"valid": true
},
{
"description": "elements may be omitted if their value is zero",
"data": "PT1H1S",
"valid": true
},
{
"description": "one and a half days, in hours",
"data": "PT36H",
"valid": true
},
{
"description": "one and a half days, in days and hours",
"data": "P1DT12H",
"valid": true
},
{
"description": "two weeks",
"data": "P2W",
"valid": true
},
{
"description": "weeks cannot be combined with other units",
"data": "P1Y2W",
"valid": false
},
{
"description": "half a year duration, with full stop decimal point",
"comment": "ISO 8601 revision in year 2000 added decimals",
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem like these apply, if I read the spec right. The spec says:

Date and time format names are derived from RFC 3339, section 5.6. The duration format is from the ISO 8601 ABNF as given in Appendix A of RFC 3339.

The ABNF there doesn't look like it allows these, no? The 2000 revision doesn't seem like it should apply.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is more useful to include the modern spec than one that is 25+ years old (I believe RFC3339 is intending to reference ISO8601 as a whole, rather than redefining it with restrictions), but this is not a hill I want to die on. I'd ask @handrews to update the spec to include the latest ISO8601 revisions, but I doubt he wants to spend another minute more thinking about formats.

Copy link
Member

Choose a reason for hiding this comment

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

I don't necessarily disagree on the spec side, but the test suite isn't the place for innovation -- if the spec does mean to include this behavior then that's all fine and we should, but if not, we need to first update the spec, and then apply the behavior to the drafts it will (in the future) apply to.

Copy link
Member

Choose a reason for hiding this comment

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

(To be clear, I think the action item here is "find out whether the spec intends to include this or not" -- and if it doesn't, we have to axe these, and then we can both add the clarification to the next spec and then at that point add these then)

But probably also easier to merge all the other indisputable tests first, so might be good to again split this PR so that those can be merged. (Or otherwise we can just keep this open until we clarify?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I created a ticket here -- json-schema-org/json-schema-spec#947 -- does this summarize the situation fully?

Copy link
Member

Choose a reason for hiding this comment

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

If we split the PR in 2 some of these that aren't related to that ticket can get merged immediately if you want? Otherwise yeah we can just leave it until the spec addresses.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed - pulled out into #412. I can rebase this PR later but will leave it alone for now so the conversation is not lost.

"data": "P0.5Y",
"valid": true
},
{
"description": "half a year duration, with comma decimal point",
"data": "P0,5Y",
"valid": true
},
{
"description": "only one unit can have a non-integer quantity",
"data": "P0.5Y2.1M",
"valid": false
},
{
"description": "only the smallest unit can have a non-integer quantity",
"data": "P0.5Y2M",
"valid": false
}
]
}
Expand Down