-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: add mandatory test 6.1.37 #218
base: 196-csaf-2.1
Are you sure you want to change the base?
Conversation
Coverage after merging 197-mandatory-test-6.1.37 into 196-csaf-2.1
Coverage Report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please
- tighten the regex as discussed,
- check leap seconds/years
- use changed path from Disclosure date oasis-tcs/csaf#879
942047f
to
c257dcb
Compare
Coverage after merging 197-mandatory-test-6.1.37 into 217-csaf-2.1-test-script
Coverage Report
|
@tschmidtb51 Okay, I addressed the issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a few details
date: { type: 'string' }, | ||
}, | ||
}, | ||
initial_release_date: { type: 'string' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the current_release_date
if (!isValidDate(date)) { | ||
ctx.errors.push({ | ||
instancePath: path, | ||
message: `invalid date`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be more specific and tell what exactly was wrong with the date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm, we could divide into two errors. One where the date does not match specified format and one where the date is invalid.
validateDate( | ||
doc.document?.tracking?.initial_release_date, | ||
'/document/tracking/initial_release_date' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing current_release_date
doc.vulnerabilities?.forEach((vulnerabiltiy, vulnerabilityIndex) => { | ||
const prefix = `/vulnerabilities/${vulnerabilityIndex}` | ||
|
||
validateDate(vulnerabiltiy.disclosure_date, `${prefix}/disclosure_date`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validateDate(vulnerabiltiy.disclosure_date, `${prefix}/disclosure_date`) | |
validateDate(vulnerabiltiy.disclosure_date, `/${prefix}/disclosure_date`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah interesting, do you have a test case where this fails? In my understanding, this would end up in two forward slashes.
['2024-01-01T00:00:00.0Z', true], | ||
['2024-01-01T00:00:00.11111111Z', true], | ||
['2024-01-01T00:00:00+01:00', true], | ||
['2024-01-01T00:00:00.111111+01:00', true], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to add a few more things here:
['2024-01-01T00:00:00.111111+01:00', true], | |
['2024-01-01T00:00:00.111111+01:00', true], | |
['2024-02-29T00:00:00.987564+01:00', true], | |
['2015-06-30T10:29:60-13:30', true], | |
['2015-06-30T23:59:60+00:00', true], | |
['2015-07-01T06:59:60+07:00', true], | |
['2016-12-31T00:00:60+23:59', true], | |
['2016-12-31T23:59:60+00:00', true], | |
['2017-01-01T02:59:60+03:00', true], | |
['2017-01-01T02:59:60+04:00', false], | |
['2024-02-30T00:00:00+01:00', false], | |
['2024-04-31T00:00:00+01:00', false], | |
['2024-13-31T00:00:00+01:00', false], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm, I'm not sure if just the boolean was flipped but many of these cases fail for me:
These dates are invalid, but marked as valid in the sample
2015-06-30T10:29:60-13:30
2015-06-30T10:29:60-13:30
2015-06-30T23:59:60+00:00
2015-07-01T06:59:60+07:00
2016-12-31T00:00:60+23:59
2016-12-31T23:59:60+00:00
2017-01-01T02:59:60+03:00
2017-01-01T02:59:60+04:00
These dates are valid, but marked as invalid in the sample
2024-02-30T00:00:00+01:00
2024-04-31T00:00:00+01:00
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm, I'm not sure if just the boolean was flipped but many of these cases fail for me:
These dates are invalid, but marked as valid in the sample
2015-06-30T10:29:60-13:30
2015-06-30T10:29:60-13:30
2015-06-30T23:59:60+00:00
2015-07-01T06:59:60+07:00
2016-12-31T00:00:60+23:59
2016-12-31T23:59:60+00:00
2017-01-01T02:59:60+03:00
2017-01-01T02:59:60+04:00
These dates are valid, but marked as invalid in the sample
2024-02-30T00:00:00+01:00
2024-04-31T00:00:00+01:00
I'm pretty sure the boolean is right. The dates covered in the valid part refer to leap seconds.
For the invalid part: According to my count the February 2024 just had 29 days, not 30 and the April 30, not 31.
Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, the dates covered in the valid part have 60
seconds which is invalid IMHO. I think in RFC 3339, 10:29:60
is expressed as 10:30:00
e.g.
For the invalid part: 2024-02-30T00:00:00+01:00
is marked with timezone offset +01:00
which converts to the following UTC
date: 2024-02-29T23:00:00Z
which is valid, since (as you noted) february had 29 days.
I hope, my understanding here is correct 🙈
c257dcb
to
35a73ff
Compare
Coverage after merging 197-mandatory-test-6.1.37 into 196-csaf-2.1
Coverage Report
|
35a73ff
to
b5d6a72
Compare
Coverage after merging 197-mandatory-test-6.1.37 into 196-csaf-2.1
Coverage Report
|
@tschmidtb51 I added the check for the |
No description provided.