CFA base #347
CFA base #347
Conversation
sadielbartholomew
left a comment
There was a problem hiding this comment.
The fix looks sound, but I think the test (though promising and better than nothing, as-is) may need adjusting, because if I take this new test_cfa.py and try it on the master branch, test_cfa_base passes, implying the bug is not being exposed by it.
|
Good point on the test. I realise that do it wholly properly we would need to move the original and cfa files to a new location and read them. I'll try and add that. |
|
How about: f4824d7? (I had to add a "u+w" chmod option so that |
Hmmm 🤔 , it's still passing for me when I copy both the |
sadielbartholomew
left a comment
There was a problem hiding this comment.
After external discussions, we've agreed that this is good enough to merge despite the remaining deficiency that:
if I take this new test_cfa.py and try it on the master branch, test_cfa_base passes, implying the bug is not being exposed by it
given the context (as part of code that won't be around for very much longer) and given that producing such a failure would be quite difficult. I am satisfied that the new unit test documents the issue at hand, which is better than nothing.
Thanks David, please merge.
Fixes #346