GatorCogitate / cogitate_tool

Tool to analyze contributions when working in a team.
5 stars 13 forks source link

Building and Testing: Features 1 & 2 #71

Closed MaddyKapfhammer closed 4 years ago

MaddyKapfhammer commented 4 years ago

This is a pull request for the first feature developed by the Testing and Building team. The separation of the PR will allow us to understand what exactly should be changed for this code specifically, and lead to a hopefully quick merge to master.

corlettim commented 4 years ago

Please make sure to resolve conflicts

enpuyou commented 4 years ago

Also, how different is this PR from #72, they share some same functions right?

lussierc commented 4 years ago

I have refactored the code previously in branches BuildingAndTesting-Feature1 & BuildingAndTesting-Feature2. The code is now combined in the branch used for this PR, BuildingAndTesting-Feature1. The feature(s) determine how many of a user's commits were going to testing and not to testing.

With this, the code refactoring changes this feature from being standalone in it's own file to working with data_collection.py which will be used by the class. As a result, these features should now be ready to merge into master and interact with the rest of the project properly. There are also test cases that have been added for the new feature's function, get_testing_commit_info().

lussierc commented 4 years ago

For your convenience, here is the output of the table printing function print_testing_in_table() which uses the data gathered by the function get_testing_commit_info().

+-----------------------+---------+--------------------+-------------------+-------------------------------+-----------------------------------+
|        Username       | Commits | Commits to Testing | Commits Elsewhere | Percent of Commits to Testing | Percent of Commits not to Testing |
+-----------------------+---------+--------------------+-------------------+-------------------------------+-----------------------------------+
| Gregory M. Kapfhammer |    17   |         0          |         17        |              0.0              |               100.0               |
|        clussier       |    41   |         1          |         40        |       2.4390243902439024      |          97.5609756097561         |
|      Juncheng Wu      |    33   |         2          |         31        |       6.0606060606060606      |         93.93939393939394         |
|      Lancaster Wu     |    23   |         0          |         23        |              0.0              |               100.0               |
|        Devin Ho       |    38   |         8          |         30        |       21.052631578947366      |         78.94736842105263         |
|   Christian Lussier   |    9    |         0          |         9         |              0.0              |               100.0               |
|     Spencer Huang     |    1    |         0          |         1         |              0.0              |               100.0               |
+-----------------------+---------+--------------------+-------------------+-------------------------------+-----------------------------------+
noorbuchi commented 4 years ago

I think this is a great addition to the data collection module, however, I have some change requests before merging considering that I am refactoring this module at the moment due to a different issue. I will let you know of these changes soon.

lussierc commented 4 years ago

I think this is a great addition to the data collection module, however, I have some change requests before merging considering that I am refactoring this module at the moment due to a different issue. I will let you know of these changes soon.

@noorbuchi When will this refactoring be done? What is being refactored? How/will will this refactoring impact my code, if at all?

We are running out of time to merge these with the project deadline coming up on Friday and Interface teams still not having features to work with. I will be merging ASAP once I have the necessary approvals, so if I can fix these issues now, that would be preferred.

codecov-io commented 4 years ago

Codecov Report

Merging #71 into master will not change coverage by %. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #71   +/-   ##
=======================================
  Coverage   65.59%   65.59%           
=======================================
  Files           2        2           
  Lines         186      186           
  Branches       35       35           
=======================================
  Hits          122      122           
  Misses         61       61           
  Partials        3        3           
noorbuchi commented 4 years ago

@lussierc I completely understand your concerns regarding the time schedule, I feel the same way and I am really trying to get the changes done ASAP. The changes have to do with the json files and the way the program reads and writes from them. Specifically, we plan on using two json files, one for the raw data and another for the individual metrics which would include the commits testing data that you have. These changes will be added soon before the end of today. I recommend that you check the multiple_json_files and individual_metrics_lines branches for more detail about these changes.

lussierc commented 4 years ago

Alright, @noorbuchi, I have refactored the code to combine the calculate_individual_metrics() and get_testing_commit_info() functions. This will also combine the printing functions.

Since I am on the WebInterface team, I am technically not supposed to work on refactoring features anymore so I will likely be stopping work on this feature now. The newly refactored code should alleviate many of your concerns but let me know if you have questions or issues. '

Since the code has been reformatted to the style currently in master (the style used by calculate_individual_metrics()), it should be easy to make any other changes you need to/are already making since the format is the same.

noorbuchi commented 4 years ago

@lussierc Thank you for working on solving this issue. I will create a PR for the changes I've been working on and then I will refactor this code to be compatible with the new changes. After all of these changes are complete, we can merge this PR if that's ok with you. Good luck with finishing the web interface! Please let me know of any updates or concerns about this branch.

lussierc commented 4 years ago

@noorbuchi Sounds good! Once you update it with your changes it should be good to merge, but let me know if you need any info/help. Thanks again for your help!

lussierc commented 4 years ago

To update everyone: I have refactored the code to combine the calculate_individual_metrics() and get_testing_commit_info() functions. The code used to calculate information about commits to testing was simply refactored and put into the calculate_individual_metrics() function to simplify our code. Similarly, I have combined the printing functions that were previously separate.

Here is an updated output (using the same example repo from earlier):

+-----------------------+-----------------------------------------------+---------+-----+-----+-------+----------------+--------------+-----------------------------------------------------------+--------------------+-------------------+-------------------------------+-----------------------------------+
|        Username       |                     Email                     | Commits |  +  |  -  | Total | Modified Lines | Lines/Commit |                         File Types                        | Commits to Testing | Commits Elsewhere | Percent of Commits to Testing | Percent of Commits not to Testing |
+-----------------------+-----------------------------------------------+---------+-----+-----+-------+----------------+--------------+-----------------------------------------------------------+--------------------+-------------------+-------------------------------+-----------------------------------+
| Gregory M. Kapfhammer |             gkapfham@allegheny.edu            |    17   | 717 |  23 |  694  |      740       |      43      |   ['', '.gradle', '.md', '.properties', '.xml', '.yml']   |         0          |         17        |              0.0              |               100.0               |
|        clussier       |             lussierc@allegheny.edu            |    41   | 644 | 189 |  455  |      833       |      20      |          ['', '.JPG', '.gradle', '.java', '.md']          |         1          |         40        |       2.4390243902439024      |          97.5609756097561         |
|      Juncheng Wu      |          wuj@aldenv138.allegheny.edu          |    33   | 705 | 176 |  529  |      881       |      26      |                    ['', '.java', '.md']                   |         2          |         31        |       6.0606060606060606      |         93.93939393939394         |
|      Lancaster Wu     | 31478979+Lancasterwu@users.noreply.github.com |    23   | 995 | 203 |  792  |      1198      |      52      | ['.java', 'Campaign', 'Experiment', 'Main', 'experiment'] |         0          |         23        |              0.0              |               100.0               |
|        Devin Ho       |          hod@aldenv139.allegheny.edu          |    38   | 485 | 131 |  354  |      616       |      16      |                    ['', '.java', '.md']                   |         8          |         30        |       21.052631578947366      |         78.94736842105263         |
|   Christian Lussier   |   32375724+lussierc@users.noreply.github.com  |    9    |  84 | 112 |  -28  |      196       |      21      |                 ['.JPG', '.md', 'LICENSE']                |         0          |         9         |              0.0              |               100.0               |
|     Spencer Huang     |   31478964+huangs1@users.noreply.github.com   |    1    |  1  |  1  |   0   |       2        |      2       |                         ['.java']                         |         0          |         1         |              0.0              |               100.0               |
+-----------------------+-----------------------------------------------+---------+-----+-----+-------+----------------+--------------+-----------------------------------------------------------+--------------------+-------------------+-------------------------------+-----------------------------------+
mcnultycollin commented 4 years ago

this looks like the could has re use potential beyond the scope of our project - this same code could easily be revised to check whether people are working on other stuff besides test cases.

noorbuchi commented 4 years ago

@lussierc I just finished working on the other two branches. One of them got merged into master but the other still has a pull request open and it still needs some changes. I will work on this pull request tomorrow and get it merged as promised.

lussierc commented 4 years ago

@noorbuchi, sounds great, thank you!

corlettim commented 4 years ago

Please make sure you resolve the branch conflicts but otherwise this looks good!!

bagashvilit commented 4 years ago

@lussierc I just saw the table with new metrics, why don't you just show the number of commits for testing and number of commits for not testing, and remove percentages, because my team is working on Individual evaluation and we already find the percentage of individual contribution, also I think that all the extra calculations should be done in the evaluation. Please let me know what you think

lussierc commented 4 years ago

@lussierc I just saw the table with new metrics, why don't you just show the number of commits for testing and number of commits for not testing, and remove percentages, because my team is working on Individual evaluation and we already find the percentage of individual contribution, also I think that all the extra calculations should be done in the evaluation. Please let me know what you think

@bagashvilit I have removed percentages while refactoring code to work with the new changes in master. Please take a look and tell me what you think.

lussierc commented 4 years ago

Hi all, I made the quick updates needed to refactor this feature in order to work with the code in master. Please let me know if you have any issues or comments, we will likely be merging this later today barring any issues.

I also did a quick update to the necessary test functions so that they will pass and work with these changes.

bagashvilit commented 4 years ago

@lussierc Thank you for the update, this looks good