search

chaoss / grimoirelab-graal

A Generic Repository AnALyzer
GNU General Public License v3.0
21 stars 62 forks source link

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

Closed inishchith closed 5 years ago

inishchith commented 5 years ago

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

Thanks!

Signed-off-by: inishchith inishchith@gmail.com

valeriocos commented 5 years ago

Is it possible to add tests @inishchith ?

inishchith commented 5 years ago

@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 commented 5 years ago

You can start having a look at the setUpClass: https://github.com/chaoss/grimoirelab-graal/blob/5dc57fab09a201e8e39908adc7dad74021b2981a/tests/test_cocom.py#L47 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 commented 5 years ago

@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 commented 5 years ago

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

Thanks :)

inishchith commented 5 years ago

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 commented 5 years ago

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).

inishchith commented 5 years ago

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

Thanks!