-
Notifications
You must be signed in to change notification settings - Fork 103
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
BUG: make time type string [s ns] #662
Conversation
Signed-off-by: FirefoxMetzger <[email protected]>
This is such an odd failure. The log seems to agree with me (full log below)
@chapulina Is this a known flakey test, or do you have any idea what could be going on? Full Test Log``` test 103 Start 103: INTEGRATION_schema_test103: Test command: /Users/jenkins/workspace/sdformat-ci-pr_any-homebrew-amd64/build/test/integration/INTEGRATION_schema_test "--gtest_output=xml:/Users/jenkins/workspace/sdformat-ci-pr_any-homebrew-amd64/build/test_results/INTEGRATION_schema_test.xml"
|
humm strange failure, only on macOS. There aren't known flakes for this branch and platform, that's why we set the check as Issues on macOS could be due to different dependency versions... Sometimes it's a clang vs gcc thing, or a timing issue (because our macOS CI machines are slow), but I don't think it could be those here. |
Okay. It could indeed be a timeout, considering that the test took ~ 1 minute to fail for such a simple thing (running xmllint should take ms, right?). I'm not a Jenkins user myself, but is the setup included in this time? If not, this could be further evidence for a timeout. |
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.
Thanks for the fix! Just a quick comment
@@ -31,7 +31,8 @@ | |||
</xsd:simpleType> | |||
|
|||
<xsd:simpleType name="time"> | |||
<xsd:restriction base="xsd:double"> | |||
<xsd:restriction base="xsd:string"> | |||
<xsd:pattern value="\d+ \d+"/> |
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.
Could you also update the others as well
<xsd:pattern value="\d+ \d+"/> | |
<xsd:pattern value="\s*\d+\s+\d+\s*"/> |
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.
@jennuine Hm, if we are worried about whitespace, wouldn't something like this be a bit more readable?
<xsd:restriction base="xsd:string">
<xsd:whiteSpace value="collapse"/>
<xsd:pattern value="\d+ \d+"/>
</xsd:restriction>
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.
Sounds good to me 👍
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.
@jennuine Done.
Signed-off-by: FirefoxMetzger <[email protected]>
🦟 Bug fix
Fixes #642
Summary
While I did this in #643 I think it makes sense to make it a separate PR and rebase. This way this bug can be fixed while awaiting feedback for different things in the other PR.
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge