-
Notifications
You must be signed in to change notification settings - Fork 8
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
Upgrade jgit to 6.9.0.202403050737-r #509
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #509 +/- ##
============================================
- Coverage 82.70% 82.28% -0.42%
Complexity 363 363
============================================
Files 43 43
Lines 1769 1722 -47
Branches 303 304 +1
============================================
- Hits 1463 1417 -46
+ Misses 180 179 -1
Partials 126 126 ☔ View full report in Codecov by Sentry. |
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.
Hi @wetneb,
Ah, I see, so the problem was that JGit added in the base chunk as well (fortunate for your current endeavors!).
I only have a little thing to say about the new unit test :)
@@ -0,0 +1,27 @@ | |||
package se.kth.spork; |
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.
I'm somewhat opposed to adding new unit-ish tests unless there's strong reason for it (which I don't see, feel free to point out something I'm missing).
While this exact scenario isn't tested anywhere, line-based merge conflicts are tested in this scenario test.
If you feel something is missing, consider either adding it to the existing test scenario I linked or creating a new scenario. Or justify why this should be unit tested.
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.
Sure, I can remove it… for me it's a habit more than anything else, as a way to document the behaviour of individual components, but for a tool like spork I can see it makes sense to rely on integration tests instead.
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 @wetneb!
Fixes #487.