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
Fix for field error handling bug #851
Conversation
1e2c347
to
f1f44fd
Compare
575ea9b
to
b47bf7e
Compare
@dgsb, this should be good to go... I intentionally kept it simple. Let me know if you'd like any changes. |
@dgsb, @sirupsen, @stevvooe, want to let me know if this is what you wanted. It just adds a test for the newly-found bug, and then adds a fix for it. The fix is simply string manipulation. (FYI, my previous refactor of the big had been inaccurate... The old behavior was that if there was a new error, it appended only the last new error to the previous error list, otherwise, it emptied the error list.) |
Hello @loren-osborn, thanks for your contribution. The fix looks correct to me. However I find the test case too complicated to be easily maintainable in the future. May I request a few changes on it ? Please look at the review. |
71865ac
to
22d6376
Compare
I am working on an additional commit to add more granular independent tests, rather than removing the outer loop. |
@dgsb, I believe I addressed all of your concerns with my test code. Please let me know if you would like me to change anything else. |
608cbb4
to
4d09609
Compare
@loren-osborn the fix is correct, but the test you've added is way too complicated to be usable when a modification on the source code cause its failure. |
I’m sorry to learn my tests are so fragile. I will work on making simple, clear, concise tests for this |
b79878f
to
6366d00
Compare
@dgsb, I don’t think I was able to simplify this very much, but think I made it a bit more clear. I also wasn’t able to find the fragility you alluded to. If you can tell me how the tests are failing, i’m happy to make them more robust. |
Just a note, the three failing commits are supposed to be failing, since the fix isn’t applied until the final commit. |
2ec0cca
to
71a8b0f
Compare
@dgsb, I took two more shots at clarifying, simplifying and fixing any fragility.
I'm open to any suggestions, but at this point I feel a bit like I'm going in circles. I'm happy with the state of the PR now, but please offer more specific feedback if you think it needs more work. Thank you, |
I just noticed I left Update: |
ecca783
to
aba5b66
Compare
I don't think I made these much simpler, but I tried to make them more clear. I was also unable to find the fragility in the tests @dgsb previously alluded to.
caf9650
to
c231c68
Compare
Rebased this on @dgsb, I would really appreciate any suggestions on how to make this suitable for merging. I made the tests as clear and simple as I was able, and am unaware of anything making them fragile. Please point any specifics you'd like me to change, and I'm happy to. |
@loren-osborn the fix itself is something like 10 lines, and all the tests around are something like 400 hundreds lines. I think it is too wide and not needed for this fix. Can you write the tests with something like 50 lines ? |
duplicate of #874 |
This is a fix to a bug when error handling resulting from serializing fields containing closure values.
@dgsb, this should be ready to review now. This is a replacement for #847. It has two distinct commits intentionally:
go test
used in Travis CI.