Skip to content
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

Feature/new specified branch commit #164

Merged

Conversation

Evleaps
Copy link
Contributor

@Evleaps Evleaps commented Sep 3, 2022

Feature/new specified branch commit

Problem:
Depends on CI configurations might be some problem with false positive affected files in AMD.

What I did?

  • SpecifiedBranchCommit using git rev-parse command for getting sha.
  • SpecifiedBranchCommit2 using git merge base command for getting sha.

What does it mean?
When we run any AMD command we compare the current branch with the specified parent branch. Consider an example when, during the development of our feature,
another developer merged his changes (9 files) into our common remote parent branch - "origin/dev".

Please, look at picture:
image

Suppose we have changed 6 files in our "feature" branch.

  1. Behaviour of SpecifiedBranchCommit:
    AMD will show the result that 15 files were affected. Because our branch is not updated (pull) and AMD will see our 6 files and 9 files that were merged by another developer.
  2. Behaviour of SpecifiedBranchCommit2:
    AMD will show the result that 6 files were affected. And this is the correct behavior.

Hence, depends on your CI settings you have to configure AMD right.

@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Merging #164 (0f6a7f2) into main (ae455b5) will increase coverage by 0.89%.
The diff coverage is 100.00%.

❗ Current head 0f6a7f2 differs from pull request most recent head 06b84e0. Consider uploading reports for the commit 06b84e0 to get more accurate results

@@             Coverage Diff              @@
##               main     #164      +/-   ##
============================================
+ Coverage     51.50%   52.39%   +0.89%     
- Complexity       72       75       +3     
============================================
  Files            13       14       +1     
  Lines           532      542      +10     
  Branches         98       99       +1     
============================================
+ Hits            274      284      +10     
  Misses          232      232              
  Partials         26       26              
Impacted Files Coverage Δ
...ectedmoduledetector/AffectedModuleConfiguration.kt 100.00% <100.00%> (ø)
...ledetector/commitshaproviders/CommitShaProvider.kt 100.00% <100.00%> (ø)
...mmitshaproviders/SpecifiedBranchCommitMergeBase.kt 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@joshafeinberg
Copy link
Member

This seems to be failing both building the sample app and the code coverage (there does not appear to be any tests in here).

This also seems to be a near duplicate of ForkCommit, can you explain the difference?

@Evleaps
Copy link
Contributor Author

Evleaps commented Oct 5, 2022

This seems to be failing both building the sample app and the code coverage (there does not appear to be any tests in here).

This also seems to be a near duplicate of ForkCommit, can you explain the difference?

Hey Josh! I added tests for the new approach!
And yes, u are right, SpecifiedBranchCommit2 same as the ForkCommit approach, but they have a big difference.
ForkCommit - compare against the commit the branch was FORKED FROM
but SpecifiedBranchCommit2- compare against the nearest ancestors with $specifiedBranch

We must save SpecifiedBranchCommit2 and Fork commit separately.

import com.dropbox.affectedmoduledetector.GitClient
import com.dropbox.affectedmoduledetector.Sha

class SpecifiedBranchCommit2(private val specifiedBranch: String) : CommitShaProvider {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please try to think of a better name for this? I understand naming is hard but adding 2 to the end doesn't explain much and makes it a bit confusing

Copy link
Contributor Author

@Evleaps Evleaps Oct 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I really agree with u. I thought about it but can't make it better.

I considered SpecifiedBranchCommitMergeBase and SpecifiedBranchCommit2
plus explanation in readme.md in both cases.

What do u think about SpecifiedBranchCommitMergeBase? Or, maybe can u suggest another name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SpecifiedBranchCommitMergeBase works for me

@joshafeinberg joshafeinberg merged commit c5143b2 into dropbox:main Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants