Allow manual merging #780
Allow manual merging #780
Conversation
|
This should be ready now, I think? I rebased the old PR and investigated and fixed the cucumber failures (with the help of cucumber/aruba#675). I also address some not yet addressed comments about the documentation from the previous PR. |
|
Well, I'm now trying this in my app and it's not quite working as I expect. I need to have a closer look, so this is back to WIP. |
|
Thanks so much @deivid-rodriguez (and @ticky !) for this 💚 🎉 🚀 As it's WIP atm I haven't taken a look yet, will take one at another time but just wanted to express my gratitude 🤗
|
|
Thanks for your encouraging and positive attitude :) I'll try to get this over the line! |
|
Thanks for pushing ahead with this @deivid-rodriguez! Your changes are looking good to me - I’ll try them out in place of my branch and see if things still work as expected. |
|
Ok, I think I fixed the issue. Previously results were being incorrectly merged using This now works as I expect against my library, I'll look into getting the last fix covered. |
|
@ticky Make sure you use the latest commit too. I think your different runners are correctly merging the set of files that they cover all together, but you might not be properly getting the set of lines covered for each file properly merged. So, I would expect the last commit to increase the total coverage in your application. |
|
All working for us here on 4fedabd. Looks like a minor change in coverage data reporting, but it seems about as expected. Nice work @deivid-rodriguez :) |
|
@ticky thanks for testing this out! 💚 Really good work @deivid-rodriguez ! I have a (minor) knee surgery tomorrow, not sure if I get to look at this properly before ~Friday/Saturday so please excuse my tardyness but I really want to get this in :)
|
|
@PragTob Hope your surgery goes well! Regarding this PR, I still want to give it another look and try a few extra things, so no rush. Will let you know when I consider it ready for review! |
|
Found one more issue where only the first command_name => coverage_data pair on each result set was getting merged. Fixed by 4d3492d. |
|
The failing test is the JRuby flaky one I gotta take care off... chose to not rerun it as I figured you still wanted to update it here |
|
Thanks for the review @PragTob! I still want to fix some issues with the current implementation. For example, a correct merged report is now generated twice, one inline, and one in the |
|
Sounds like a plan! 👌
|
|
So I created some unit tests for this, but I run into several issues while creating them that I think might be related to #820. So I think I might've fixed that issue? What I did was essentially making sure that when reading reports from disk into a hash, or manipulating them, all keys are always symbols. Do we have an regression scenario for #820? If so, I can extract the relevant changes from this PR into a separate PR, and add that regression test in there. |
|
@deivid-rodriguez is the last ticket reference to #802 a mistake and you wanted to do #820 ? If so then we almost have one, I started writing one (generated an old resultset.json, created a project for it etc) but then got sick and took a break 😁 If I don't feel worse tomorrow I'll likely finish it up tomorrow and will check against your branch. Although, imo it's likely that this isn't #820 as I think #820 has to do with the different file structure (extra level) not the keys being symbols. On a slightly related note I was pondering removing the "symbolization" of keys as it seems to be a waste of memory & run time while we could us them without them being symbolized but I wanted to double check and see how far the symbolization goes. |
|
Yes, I corrected the mistake now. I'm not sure it wasn't #820, but since I ended up removing the Regarding removing the symbolization, I think that's a good move, yeah. I was about to do that to fix the issue I was having (some "file name" keys were symbols, and others string, so they wouln't merge fine). But I ended up symbolizing everywhere instead, since it looked simpler to achieve. |
|
Oh, I also made this change: 93c771d, because |
|
Yeah it crashed in the symbolization, but I think it'll then just crash somewhere else where it tries to traverse the tree - unless the time is old (meaning time in the resultset is after the timeout) in which case it wouldn't do that and I'd have to adjust my test :D Not sure about the other commit, I'd have to check impact in other places. I think it's more in line with what normal Coverage returns so that's good - sometimes I prefer to have all keys but just have them be empty but... tradeoffs 🤷♂️ |
|
@deivid-rodriguez so me trying to reproduce the failure is at the branch edit: yeah right now once we get to the code that blows up the old resultset.json had been overridden with a new one already 🤷♀️ edit2: ok if I don't let it store the result then it crashes like it does in the report, now to figure how to make that. I probably should comment on #820 though ;) |
|
@deivid-rodriguez Okay I double checked with #824 and it first it seemed that it solved the crash, but it then highlighted a couple of errors in my test setup that I need to fix up (the paths in the saved coverage.json need to be the same ones as the local machine and I need to update the time to not go beyond the merge timeout as your changes move the crash to that time). Thanks! 💚 |
There was a problem hiding this comment.
Thanks a lot! Looking great overall! My comments are very minor.
@deivid-rodriguez I'm wondering what to do with this - do you think it's ready for merge? I mean we're "only" at beta releases right now any way. I'd like to merge it because it touches/alleviates some of the problems of #820 and so if I also did something in #824 (which is blocking a beta2 release right now) it'd most likely be a waste of time + merge problems for either your or me. None of which seem appealing to me right now. So I'd rather merge it sooner than later if you think it's cool :)
|
@PragTob I have no problem with this getting merged now, although it contains a bunch of commits which seem completely unrelated to the coverage merging (although I did add them because of issues when getting the unit specs to pass, so I guess there's some relation after all). So, your choice, I don't mind extracting those commits to a separate PR, and I don't care about rebasing stuff and fixing merge conflicts. Just let me know. Regarding your feedback, yeah, I just wanted to push bare passing specs but I was planning to refactor and improve them afterwards. I'll get this ready for you later today hopefully. I'd also like to do some commit history cleanup since there's a lot of stuff in here that can be squashed together, but I can also skip doing that if you prefer the original verbose git history of this PR. |
|
@deivid-rodriguez Thanks 💚 All volunteers here so no worries, but if you managed to do it by this evening that'd be super great. I'll leave my work on #824 alone until it's done or tomorrow morning or so then. I'd appreciate a bit of squashing (not strictly necessary from my side but some fewer seems like a nice idea) :) While I sympathize with extracting it to a separate PR I think it's fine to have in here - these things sometimes escalate and happen but imo it's small enough (and I've already reviewed it anyhow) to keep in here imo :) Thanks heaps!
|
|
I addressed your feedback. If you're good with the current version I can do a pass of squashing. |
|
@deivid-rodriguez I'm good with it, ruby 2.4.9 seems to have a legit failure though 😱 Anyhow, thanks a bunch for the work! 💚 |
|
It seems like a flaky... :( |
|
I can reproduce locally using the same test seed, will have a look. |
|
It should be ok now. I'll do the squashing. |
|
I think I detected a potential bug while squashing, I'll have a closer look later. |
|
Ok, so I pushed a cleaner git history, which consists of the following:
|
|
That "potential bug" was a false alarm by the way 😅. |
|
It's a flaky world! I'll have a look at the new failure :) |
|
Yay flakies! 💃 🙄 Good luck with those. I guess instead of waiting I might just rebase my branch against this one and work from there, might be the most efficient so close to the finish line. Also btw you're probably better equipped than me to write the Changelog (doesn't need to be within this PR, can be separate) Full symbolization helps with #820 but doesn't fully fix it (it helps for the case where the report is outdated) - fixing it will sadly be somewhat "dirty" or at least that's the only way I know how to (rescuing errors from the code that does the actual merging, print a warning, return empty value - while not great this helps us to not repeat this once we change the format again) Cheers and a bunch of thanks! |
For example, if `SimpleCov.run_exit_tasks!` has been called explicitly, we don't want to call it again. This is a useful feature for testing the collation feature that I'll be adding in follow up commits, but also has other use cases. Co-authored-by: Dan Macumber <dan.macumber@gmail.com> Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Dirty tests should clean up after themselves, not cleanup other dirty specs leaks. In this case, the specs were cleaning up dirty state before them, so they could work, but leaking state after being running.
Branch coverage needs to be enabled for this test to properly run.
So that cucumber scenarios never depend on having to properly set the $LOAD_PATH.
|
I fixed the flaky and wrote changelog entries! |
|
Thanks so much for your work getting this over the line! Super excited for this to hit a release :) |
|
@ticky it's live now as 0.18.0.beta2 - if you could give it a try that'd be highly appreciated! :) |
|
Working great for us! Loving it :) |
|
Happy to hear it!
|
This my WIP version of #681. It would probably be better to keep the discussion in that PR, but to do that I would need permissions to push to @ticky's fork.