feat: new experience to get coverage report #1785

AkashKumar7902 · 2024-04-06T13:32:06Z

Related Issue

Closes: #1735

Describe the changes you've made

Add coverage data to test report
coverage data will be appended for applications written in:

golang
node
python
java

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, local variables)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Please let us know if any test cases are added

Please describe the tests(if any). Provide instructions how its affecting the coverage.

Describe if there is any unusual behaviour of your code(Write `NA` if there isn't)

A clear and concise description of it.

Checklist:

My code follows the style guidelines of this project.
I have performed a self-review of my own code.
I have commented my code, particularly in hard-to-understand areas.
I have made corresponding changes to the documentation.
My changes generate no new warnings.
I have added tests that prove my fix is effective or that my feature works.
New and existing unit tests pass locally with my changes.

Screenshots (if any)

Original	Updated
original screenshot	updated screenshot

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

pkg/service/tools/coverage.go

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

pkg/service/tools/coverage.go

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

…nge NYC_CLEAN to CLEAN Signed-off-by: Akash Kumar <meakash7902@gmail.com>

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

…nction Signed-off-by: Akash Kumar <meakash7902@gmail.com>

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

charankamarapu

Please address the comments, I haven't yet reviewed complete logic but these are the initial comments.

charankamarapu · 2024-05-20T09:45:43Z

cli/provider/cmd.go

@@ -214,12 +214,12 @@ func (c *CmdConfigurator) AddFlags(cmd *cobra.Command) error {
 			cmd.Flags().Uint64("apiTimeout", c.cfg.Test.APITimeout, "User provided timeout for calling its application")
 			cmd.Flags().String("mongoPassword", c.cfg.Test.MongoPassword, "Authentication password for mocking MongoDB conn")
 			cmd.Flags().String("coverageReportPath", c.cfg.Test.CoverageReportPath, "Write a go coverage profile to the file in the given directory.")
-			cmd.Flags().StringP("language", "l", c.cfg.Test.Language, "application programming language")
+			cmd.Flags().StringP("language", "l", c.cfg.Test.Language, "Application programming language")


attaching to enum, I mean only few values should be allowed like GO, PYTHON etc and provide example in the comment.

https://github.com/thediveo/enumflag

should I use this library for enum ?

see how others do it in the case of cobra tool.

may be something like allowed values may be list of string.

Like any of these languages should be allowed if user provides other value. It should prompt that only these values will be allowed

cli/provider/cmd.go

charankamarapu · 2024-05-20T10:44:52Z

cli/provider/cmd.go

+		if cmd.Name() == "record" {
+			c.cfg.Test.SkipCoverage = true
+		}
+		if c.cfg.Test.Language == "python" && !c.cfg.Test.SkipCoverage {


use switch condition rather than if statements. define language as an enum if needed.

cli/provider/cmd.go

charankamarapu · 2024-05-20T11:54:50Z

pkg/graph/schema.resolvers.go

@@ -128,7 +128,7 @@ func (r *queryResolver) TestSetStatus(ctx context.Context, testRunID string, tes
 }

 // StopApp is the resolver for the stopApp field.
-func (r *mutationResolver) StopApp(_ context.Context, appId int) (bool, error) {
+func (r *mutationResolver) StopApp(ctx context.Context, appId int) (bool, error) {


remove graphql support as it is not used

should I remove the entire pkg/graph folder ?

charankamarapu · 2024-05-20T12:11:39Z

pkg/service/replay/replay.go

@@ -118,16 +125,47 @@ func (r *Replayer) Start(ctx context.Context) error {
 		return fmt.Errorf(stopReason)
 	}

+	if !r.config.Test.SkipCoverage && r.config.Test.Language == "typescript" {


why is this done twice..?

You can add clean, append and testset envs irrespective to languages.

this is because first clean must be set to true to clean up previous coverage data and then after first test-set has completed execution clean would be set to false

A quick solution would be to not append --clean=${CLEAN} in cli/provider/cmd.go and append it in replay.go when we are setting clean env to false(like --clean=false, no need to set env also), but it won't work because command gets saved to app struct through NewApp func invocation. should I implement an updateCmd function in app.go to clean up this code from replay.go ?

No what I am telling is this condition r.config.Test.Language == "typescript" is not required. You can just do it by default without condition, even the test-set name env.

pkg/service/replay/replay.go

charankamarapu · 2024-05-20T12:16:30Z

pkg/service/tools/coverage.go

+	"go.keploy.io/server/v2/pkg/models"
+)
+
+func CalGoCoverage() (models.TestCoverage, error) {


why are these in tools service.?

coverage calculation by analyzing the raw coverage files may be viewed as a tool, thats why I put it here, do you want to move it to a different place ?

move it into utils in test service

charankamarapu · 2024-05-20T12:37:20Z

Please resolve the pipeline failure too.

> move coverage.go from tools to service/replay pkg > define enum for language flag > refactored code Signed-off-by: Akash Kumar <meakash7902@gmail.com>

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

charankamarapu

Please address the comments

charankamarapu · 2024-05-27T06:25:54Z

cli/provider/cmd.go

+	language, executable := utils.DetectLanguage(logger, conf.Command)
+	// if language is not provided through flag/config and language detected is not unknown
+	// then set the language to detected language
+	if conf.Test.Language == "" {


I think if language is specified we need to find executable according to the language again is what I feel. Lets take the case of node. If I specified language as python and command I gave is npm, then you will consider executable as npm and change it to cover right. Try clean this approach a bit and try all possible things and also if something is missing log a warn message.

does this language detected through keploy should be given more preference over user passed language

no Want I mean is if language given and language parsed is not same we should give an error

charankamarapu · 2024-05-27T06:34:50Z

cli/provider/cmd.go

+			logger.Warn("coverage tool not found. please install coverage tool using 'pip install coverage'")
+		} else {
+			utils.CreatePyCoverageConfig(logger)
+			conf.Command = strings.Replace(conf.Command, executable, "coverage run $APPEND --data-file=.coverage.keploy", 1)


don't replace it with conf.command may be create another element conf.coverageCommand and then use it. Because we will or may log the command in the test service then We dont want to show this new command. We need to show the old command.

charankamarapu · 2024-05-27T06:38:55Z

cli/test.go


 			cmdType := utils.FindDockerCmd(cfg.Command)
-			if cmdType == utils.Native && cfg.Test.GoCoverage {
+			if cmdType == utils.Native && !cfg.Test.SkipCoverage {


why do we need this GOCOVERDIR here..? cant we do this in preprocessing..?

charankamarapu · 2024-05-27T06:45:41Z

pkg/service/replay/replay.go

+		}
+	}
+
+	jacocoPath := ""


you can do this in preprocess right..?

charankamarapu · 2024-05-27T06:46:56Z

pkg/service/replay/replay.go

@@ -117,17 +117,47 @@ func (r *Replayer) Start(ctx context.Context) error {
 		}
 		return fmt.Errorf(stopReason)
 	}
+	if !r.config.Test.SkipCoverage {


if there is an error in the preprocessing or detecting language just make skipCoverage = true

I believe the code part referenced here is doing the same

yeah I was just reminding to do it everywhere even in preprocess function.

charankamarapu · 2024-05-27T06:49:37Z

pkg/service/replay/replay.go

+	}
+
+	jacocoPath := ""
+	if r.config.Test.Language == models.Java && !r.config.Test.SkipCoverage {


we should do this only if the jacoco jar is not present in the path we set or the path which user has specified right..?

if we want jacoco commands to be ran directly not with mvn, we must use jacoco cli, jacoco cli can't be downloaded through regular dependencies in pom.xml (as its not a dependency, but rather a package)

oh ok I thought it was jacoco agent jar

charankamarapu · 2024-05-27T06:54:55Z

pkg/service/replay/replay.go

+			}
+		}
+	}
+	if !r.config.Test.SkipCoverage && r.config.Test.Language == models.Java {


we can do this after all test-sets are done running at the end.

currently its at the end of test set iteration for loop so all the tests are ran, before merge and report commands are run.

charankamarapu · 2024-05-27T06:55:59Z

pkg/service/replay/replay.go

@@ -634,7 +694,7 @@ func (r *Replayer) printSummary(ctx context.Context, testRunResult bool) {
 		}
 		r.logger.Info("test run completed", zap.Bool("passed overall", testRunResult))

-		if utils.CmdType(r.config.CommandType) == utils.Native && r.config.Test.GoCoverage {
+		if utils.CmdType(r.config.CommandType) == utils.Native && !r.config.Test.SkipCoverage && r.config.Test.Language == models.Go {


we can remove this as we are already storing the coverage in report.

charankamarapu · 2024-05-27T07:01:21Z

pkg/service/replay/utils.go

+	// Specify the output file
+	args = append(args, "--destfile", "target/keploy-e2e.exec")
+
+	cmd := exec.CommandContext(ctx, args[0], args[1:]...)


make sure we dont show these logs to user, because all we want for them to see is application and keploy logs may be print them as debug logs

should I change all current error logs introduced in this PR which are related to coverage thing to Warn ?

yes and the success logs should not be shown like if you are mergin the jacoco exec files I dont want user to see the logs because they are irrelevant to the user.

Should I print a warn statement saying to check debug logs if skipcoverage is set to true by keploy?

No if there is an error in any coverage related thing log that as warn and if there is any logs from the commands you are running like jaoco merge or report dont show the logs to user log them as debug thats it

> change error logs of coverage related tasks to warn, and error logs of coverage tool to debug > refactor > don't change command instead created a new field in config struct named CoverageCommand in which command prepended with coverage tool is put > log error if language detected doesn't match with language provided Signed-off-by: Akash Kumar <meakash7902@gmail.com>

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

feat: add coverage data for go in report

de85b50

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

AkashKumar7902 marked this pull request as draft April 6, 2024 13:32

AkashKumar7902 added 7 commits April 6, 2024 22:50

feat: add coverage data in report in case of python

a416b23

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

feat: fix golint error

4555c84

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

fix: golint issue

c80852a

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

fix: golint error

7144ab6

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

feat: add typescript code coverage to report

de16718

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

feat: complete typescript coverage update in report

28c5a8a

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

fix: build

c9b04ba

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

AkashKumar7902 changed the title ~~feat: add coverage data for go in report~~ feat: add coverage data in report Apr 7, 2024

AkashKumar7902 added 2 commits April 11, 2024 00:51

feat: add coverage data to report in case of java application

7765a1d

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

feat: add jacoco go package in go.mod

cec7ffa

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

AkashKumar7902 marked this pull request as ready for review April 10, 2024 19:22

Merge branch 'main' into add-coverage-data-to-report

905f7c5

github-advanced-security bot found potential problems Apr 10, 2024

View reviewed changes

pkg/service/tools/coverage.go Fixed Show fixed Hide fixed

AkashKumar7902 added 2 commits April 11, 2024 01:00

fix: linter issue

dbc5b04

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

fix: golint issue

be98367

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

github-advanced-security bot found potential problems Apr 10, 2024

View reviewed changes

pkg/service/tools/coverage.go Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Apr 10, 2024

View reviewed changes

pkg/service/tools/coverage.go Fixed Show fixed Hide fixed

AkashKumar7902 and others added 11 commits April 15, 2024 13:27

fix: coverage percentage

810e9ce

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

Merge branch 'main' into add-coverage-data-to-report

bc562d2

feat: remove sdk (python, typescript)

c0c6fb3

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

finalize typescript coverage

b71ac6e

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

fix: remove unused functions

ca18f92

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

fix: remove unused dependency

89b7266

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

fix: remove goCoverage flag in favour of single coverage flag and cha…

5b168d3

…nge NYC_CLEAN to CLEAN Signed-off-by: Akash Kumar <meakash7902@gmail.com>

fix: change NYC_CLEAN to CLEAN env

83d3de1

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

fix: go coverage issue

3f42936

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

append coverage tool commands to app cmd

f74e514

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

add coveragerc file in cur dir

5242037

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

AkashKumar7902 changed the title ~~feat: add coverage data in report~~ feat: new experience to get coverage report May 15, 2024

AkashKumar7902 added 3 commits May 15, 2024 09:12

remove commented lines

a62e66d

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

append jacoco path to cmd and wrote downloadAndExtractJacocoBinary fu…

c945888

…nction Signed-off-by: Akash Kumar <meakash7902@gmail.com>

add merge and report of jacoco

450052a

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

AkashKumar7902 force-pushed the add-coverage-data-to-report branch from bb312ab to 450052a Compare May 20, 2024 10:10

AkashKumar7902 and others added 4 commits May 20, 2024 15:41

Merge branch 'main' into add-coverage-data-to-report

f10bb7c

change html report dir path

e82c796

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

calculate java coverage and insert report based on csv file

a6a0741

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

handle error in case of atoi and prepend packagename in class

a46cfe5

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

charankamarapu reviewed May 20, 2024

View reviewed changes

AkashKumar7902 added 4 commits May 22, 2024 20:40

address review comments:

a350abf

> move coverage.go from tools to service/replay pkg > define enum for language flag > refactored code Signed-off-by: Akash Kumar <meakash7902@gmail.com>

remove graph package

c61d75a

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

fix: wrap merge and generate jacoco report in if condition

3e8f35c

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

give more informative log when go binary is not built with cover flag

9d78662

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

charankamarapu reviewed May 27, 2024

View reviewed changes

AkashKumar7902 added 3 commits May 27, 2024 15:56

Merge branch 'main' into add-coverage-data-to-report

c877379

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

resolve warning

d917ebf

Signed-off-by: Akash Kumar <meakash7902@gmail.com>

feat: new experience to get coverage report #1785

Are you sure you want to change the base?

feat: new experience to get coverage report #1785

Conversation

AkashKumar7902 commented Apr 6, 2024 • edited

Related Issue

Describe the changes you've made

Type of change

Please let us know if any test cases are added

Describe if there is any unusual behaviour of your code(Write NA if there isn't)

Checklist:

Screenshots (if any)

charankamarapu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charankamarapu May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charankamarapu commented May 20, 2024 • edited

charankamarapu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkashKumar7902 commented Apr 6, 2024 •

edited

Describe if there is any unusual behaviour of your code(Write `NA` if there isn't)

charankamarapu May 22, 2024 •

edited

charankamarapu commented May 20, 2024 •

edited