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

get code coverage in tests #90

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

get code coverage in tests #90

wants to merge 4 commits into from

Conversation

laverya
Copy link
Member

@laverya laverya commented May 17, 2022

No description provided.

Copy link

@label-checker label-checker bot left a comment

Choose a reason for hiding this comment

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

Label type:: is required.

@github-actions
Copy link

Total Coverage: 73.30%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
pkg/migrate
   migrate.go100%100%73.20%100, 101, 1016, 1017, 102, 1023, 1024, 103, 1030, 1031, 1045, 1046, 105, 1051, 1052, 106, 107, 108, 1084, 1085, 109, 110, 115, 116, 117, 118, 120, 121, 122, 123, 124, 127, 128, 138, 139, 169, 170, 184, 185, 217, 218, 225, 226, 234, 235, 261, 262, 263, 264, 265, 266, 267, 270, 271, 272, 276, 284, 291, 292, 295, 306, 307, 308, 315, 316, 394, 395, 396, 409, 410, 429, 430, 449, 450, 460, 461, 465, 466, 472, 473, 484, 485, 486, 49, 50, 51, 511, 512, 52, 524, 525, 53, 54, 55, 56, 57, 574, 575, 58, 581, 582, 583, 584, 585, 586, 587, 588, 589, 59, 590, 591, 599, 60, 600, 605, 61, 62, 622, 623, 624, 625, 626, 627, 628, 629, 63, 630, 631, 632, 64, 640, 641, 646, 65, 66, 661, 662, 68, 69, 690, 691, 70, 71, 714, 715, 72, 734, 735, 74, 75, 755, 756, 76, 760, 761, 764, 765, 767, 768, 77, 772, 773, 78, 79, 793, 794, 795, 796, 80, 810, 811, 818, 819, 84, 844, 845, 85, 850, 851, 86, 860, 861, 868, 869, 87, 874, 875, 88, 884, 885, 90, 901, 902, 905, 906, 91, 919, 92, 920, 93, 937, 938, 944, 945, 949, 95, 950, 956, 957, 96, 961, 962, 97, 973, 976, 977, 98, 986, 989, 990
pkg/version
   version.go100%100%100%

@github-actions
Copy link

Total Coverage: 73.30%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
pkg/migrate
   migrate.go100%100%73.20%100, 101, 1016, 1017, 102, 1023, 1024, 103, 1030, 1031, 1045, 1046, 105, 1051, 1052, 106, 107, 108, 1084, 1085, 109, 110, 115, 116, 117, 118, 120, 121, 122, 123, 124, 127, 128, 138, 139, 169, 170, 184, 185, 217, 218, 225, 226, 234, 235, 261, 262, 263, 264, 265, 266, 267, 270, 271, 272, 276, 284, 291, 292, 295, 306, 307, 308, 315, 316, 394, 395, 396, 409, 410, 429, 430, 449, 450, 460, 461, 465, 466, 472, 473, 484, 485, 486, 49, 50, 51, 511, 512, 52, 524, 525, 53, 54, 55, 56, 57, 574, 575, 58, 581, 582, 583, 584, 585, 586, 587, 588, 589, 59, 590, 591, 599, 60, 600, 605, 61, 62, 622, 623, 624, 625, 626, 627, 628, 629, 63, 630, 631, 632, 64, 640, 641, 646, 65, 66, 661, 662, 68, 69, 690, 691, 70, 71, 714, 715, 72, 734, 735, 74, 75, 755, 756, 76, 760, 761, 764, 765, 767, 768, 77, 772, 773, 78, 79, 793, 794, 795, 796, 80, 810, 811, 818, 819, 84, 844, 845, 85, 850, 851, 86, 860, 861, 868, 869, 87, 874, 875, 88, 884, 885, 90, 901, 902, 905, 906, 91, 919, 92, 920, 93, 937, 938, 944, 945, 949, 95, 950, 956, 957, 96, 961, 962, 97, 973, 976, 977, 98, 986, 989, 990
pkg/version
   version.go100%100%100%

@laverya laverya marked this pull request as ready for review May 18, 2022 06:59
@github-actions
Copy link

Total Coverage: 73.37%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
pkg/migrate
   migrate.go100%100%73.27%100, 101, 1019, 102, 1020, 1026, 1027, 103, 1033, 1034, 1048, 1049, 105, 1054, 1055, 106, 107, 108, 1087, 1088, 109, 110, 115, 116, 117, 118, 120, 121, 122, 123, 124, 127, 128, 138, 139, 169, 170, 184, 185, 217, 218, 225, 226, 234, 235, 261, 262, 263, 264, 265, 266, 267, 270, 271, 272, 276, 284, 291, 292, 295, 306, 307, 308, 315, 316, 394, 395, 396, 409, 410, 428, 429, 452, 453, 463, 464, 468, 469, 475, 476, 487, 488, 489, 49, 50, 51, 514, 515, 52, 527, 528, 53, 54, 55, 56, 57, 577, 578, 58, 584, 585, 586, 587, 588, 589, 59, 590, 591, 592, 593, 594, 60, 602, 603, 608, 61, 62, 625, 626, 627, 628, 629, 63, 630, 631, 632, 633, 634, 635, 64, 643, 644, 649, 65, 66, 664, 665, 68, 69, 693, 694, 70, 71, 717, 718, 72, 737, 738, 74, 75, 758, 759, 76, 763, 764, 767, 768, 77, 770, 771, 775, 776, 78, 79, 796, 797, 798, 799, 80, 813, 814, 821, 822, 84, 847, 848, 85, 853, 854, 86, 863, 864, 87, 871, 872, 877, 878, 88, 887, 888, 90, 904, 905, 908, 909, 91, 92, 922, 923, 93, 940, 941, 947, 948, 95, 952, 953, 959, 96, 960, 964, 965, 97, 976, 979, 98, 980, 989, 992, 993
pkg/version
   version.go100%100%100%

Copy link

@alexanderjophus alexanderjophus left a comment

Choose a reason for hiding this comment

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

lgtm, just curious about one thing


- uses: actions/upload-artifact@v2
with:
name: coverage.out

Choose a reason for hiding this comment

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

Question: What does this do? I thought jobs ran sequentially, so adding coverage here wouldn't do anything as there's no coverage.out yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Coverage.out is produced by 'make test'

Choose a reason for hiding this comment

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

You're completely right 🤦

@github-actions
Copy link

Total Coverage: 73.37%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
pkg/migrate
   migrate.go100%100%73.27%100, 101, 1019, 102, 1020, 1026, 1027, 103, 1033, 1034, 1048, 1049, 105, 1054, 1055, 106, 107, 108, 1087, 1088, 109, 110, 115, 116, 117, 118, 120, 121, 122, 123, 124, 127, 128, 138, 139, 169, 170, 184, 185, 217, 218, 225, 226, 234, 235, 261, 262, 263, 264, 265, 266, 267, 270, 271, 272, 276, 284, 291, 292, 295, 306, 307, 308, 315, 316, 394, 395, 396, 409, 410, 428, 429, 452, 453, 463, 464, 468, 469, 475, 476, 487, 488, 489, 49, 50, 51, 514, 515, 52, 527, 528, 53, 54, 55, 56, 57, 577, 578, 58, 584, 585, 586, 587, 588, 589, 59, 590, 591, 592, 593, 594, 60, 602, 603, 608, 61, 62, 625, 626, 627, 628, 629, 63, 630, 631, 632, 633, 634, 635, 64, 643, 644, 649, 65, 66, 664, 665, 68, 69, 693, 694, 70, 71, 717, 718, 72, 737, 738, 74, 75, 758, 759, 76, 763, 764, 767, 768, 77, 770, 771, 775, 776, 78, 79, 796, 797, 798, 799, 80, 813, 814, 821, 822, 84, 847, 848, 85, 853, 854, 86, 863, 864, 87, 871, 872, 877, 878, 88, 887, 888, 90, 904, 905, 908, 909, 91, 92, 922, 923, 93, 940, 941, 947, 948, 95, 952, 953, 959, 96, 960, 964, 965, 97, 976, 979, 98, 980, 989, 992, 993
pkg/version
   version.go100%100%100%

@github-actions
Copy link

Total Coverage: 73.37%

Coverage Report
FileBranchesFuncsLinesUncovered Lines
pkg/migrate
   migrate.go100%100%73.27%100, 101, 1019, 102, 1020, 1026, 1027, 103, 1033, 1034, 1048, 1049, 105, 1054, 1055, 106, 107, 108, 1087, 1088, 109, 110, 115, 116, 117, 118, 120, 121, 122, 123, 124, 127, 128, 138, 139, 169, 170, 184, 185, 217, 218, 225, 226, 234, 235, 261, 262, 263, 264, 265, 266, 267, 270, 271, 272, 276, 284, 291, 292, 295, 306, 307, 308, 315, 316, 394, 395, 396, 409, 410, 428, 429, 452, 453, 463, 464, 468, 469, 475, 476, 487, 488, 489, 49, 50, 51, 514, 515, 52, 527, 528, 53, 54, 55, 56, 57, 577, 578, 58, 584, 585, 586, 587, 588, 589, 59, 590, 591, 592, 593, 594, 60, 602, 603, 608, 61, 62, 625, 626, 627, 628, 629, 63, 630, 631, 632, 633, 634, 635, 64, 643, 644, 649, 65, 66, 664, 665, 68, 69, 693, 694, 70, 71, 717, 718, 72, 737, 738, 74, 75, 758, 759, 76, 763, 764, 767, 768, 77, 770, 771, 775, 776, 78, 79, 796, 797, 798, 799, 80, 813, 814, 821, 822, 84, 847, 848, 85, 853, 854, 86, 863, 864, 87, 871, 872, 877, 878, 88, 887, 888, 90, 904, 905, 908, 909, 91, 92, 922, 923, 93, 940, 941, 947, 948, 95, 952, 953, 959, 96, 960, 964, 965, 97, 976, 979, 98, 980, 989, 992, 993
pkg/version
   version.go100%100%100%

@erquhart
Copy link

We've made determinations in the past not to use test coverage as a metric. cc/ @marccampbell

@laverya
Copy link
Member Author

laverya commented May 21, 2022

That's fair. What I'd really like to have in this is "these are the lines you added, and this is which of those lines are covered" but that might take some actual work to implement.

@marccampbell
Copy link
Member

Agreeing with @erquhart. I'd like to not include this. In the end, it's not important that every line of code have test coverage, it's important that workflows do. Unfortunately, mapping this to "lines of code" is that best we can do today, but that comes with downsides we'd like to avoid (at least for now).

I like the goal here (give more data to the reviewer about the quality of the change). But is there a way we can do it without reporting on "lines of code covered"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants