-
Notifications
You must be signed in to change notification settings - Fork 23
Fix for umversion in cf.read
#307
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
Conversation
|
The |
|
Hi @davidhassell, the CI jobs look like they are hanging here (over 4 hours runtime) so I suggest cancelling them. I have seen these tests hanging recently and need to investigate that, and given that a few have passed enough for us to be confident this is good along with a local full-suite pass, it seems highly unlikely they are hanging due to this PR. So I'll ignore the CI in my reviewing. Thanks. |
|
Hi Sadie, the CI jobs had finally dies by the time a looked )or perhaps you killed them?). I must confess to usually running the tests individually these days, as I still get sporadic hanging - and they passed in this way for me on this PR. Do you think that moving to |
sadielbartholomew
left a comment
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.
One minor issue and a question reported in-line, but overall this is good to merge: fixes the issue at hand in the most sensible way and the tests pass locally, including the suitable new unit test.
Ah, sorry about this and the inconvenience. Something is up and it may just be in the Actions infrastructure. Once the machine and performance release is out, maybe I can briefly investigate.
I think that is much more likely to be about some specific issue(s) with the codebase or underlying C libraries rather than the test framework used, but I think moving to |
|
This is good to go so I will merge, just to tie up loose ends before the weekend... |
Fixes #306