Skip to content

[analyzer] Fix results for deleted files for CoCom backend #37

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

Merged
merged 1 commit into from
Jun 18, 2019

Conversation

inishchith
Copy link
Contributor

@valeriocos Please have a look when convenient and let me know for any improvement that could be done.

Thanks!

Signed-off-by: inishchith [email protected]

@valeriocos
Copy link
Member

Is it possible to add tests @inishchith ?

@inishchith
Copy link
Contributor Author

inishchith commented Jun 12, 2019

@valeriocos As this requires deleted files in the version history, I'm not sure how do we proceed with adding tests for the changes.
If you could give a reference idea then I would work on it and would be of great help.

Edit:

  1. I'll have a look at the possibilities till then.
  2. I'll have to make changes to graaltest.zip file and add some deletion history to it. That should work i feel

@valeriocos
Copy link
Member

You can start having a look at the setUpClass:

def setUpClass(cls):

It loads the repo available at: https://github.com/chaoss/grimoirelab-graal/tree/master/tests/data/graaltest.zip.

To test your code, you can either create a new repo with some commits or add more commits to the existing one.

A similar approach is also used in the tests for the Perceval git backend:
https://github.com/chaoss/grimoirelab-perceval/blob/master/tests/test_git.py

@inishchith
Copy link
Contributor Author

@valeriocos Thanks for the help with a quick response!.
Yes, after my comment, I realized that could be done.
I'm working on it, will update the PR soon :)

@inishchith
Copy link
Contributor Author

@valeriocos I've updated the PR with tests. Please do have a look when you get time.

Thanks :)

Copy link
Member

@valeriocos valeriocos left a comment

Choose a reason for hiding this comment

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

Thank you @inishchith for the PR, overall it looks great. I have a couple of minor comments/questions:

  • Why the values of deleted/renamed files are set to 0 and not to None ?
  • Could you add some code in a test to check that the files deleted or renamed have their values properly set ?
  • Could you squash the commits in just one and provide more details in the commit message ?
  • Please increase the version of the cocom backend

Thanks

@inishchith
Copy link
Contributor Author

Why the values of deleted/renamed files are set to 0 and not to None ?

These files might have a history (analysis data in previous commits) and in case the comparison happens at the time of visualization at a point, 0 can be assigned instead of raising an exception on None. If you agree that a None can be handled, I'll make the changes. :)

Could you add some code in a test to check that the files deleted or renamed have their values properly set ?

Sorry, I missed that. Thanks for pointing it out!.

Could you squash the commits in just one and provide more details in the commit message ?
Please increase the version of the cocom backend

Sure!.

@valeriocos I'll get back once I've worked on the proposed changes.
Thanks for reviewing:)

@valeriocos
Copy link
Member

Thank you for the quick reply @inishchith

These files might have a history (analysis data in previous commits) and in case the comparison happens at the time of visualization at a point, 0 can be assigned instead of raising an exception on None. If you agree that a None can be handled, I'll make the changes. :)

I would go for None, because zero may lead to a wrong guess (files may be empty and they will have the same cocom values of deleted files).

There can be two scenarios, a deleted file and other renamed (or the one
which has changed the directory). These changes handles both the cases.
Alter test data to accomodate cases for deleted files results

Signed-off-by: inishchith <[email protected]>
@inishchith
Copy link
Contributor Author

@valeriocos I've made the required changes. Let me know if there are any more changes to be made.

Thanks!

Copy link
Member

@valeriocos valeriocos left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @inishchith and sorry for the late reply

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