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.

Sign up for GitHub

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

Jump to bottom

[DEVHAS-490] Mocking CDQ Util for Private Repo Testing #411

Merged
merged 4 commits into from
Nov 14, 2023
Merged

[DEVHAS-490] Mocking CDQ Util for Private Repo Testing #411

merged 4 commits into from
Nov 14, 2023

Conversation

maysunfaisal
Copy link
Member

@maysunfaisal maysunfaisal commented Nov 9, 2023 •
edited
Loading

What does this PR do?:

  • Introduces CDQUtil interface to abstract out CDQ logic which utilizes a git token, so that we can mock valid cases for tests with a private repo
  • Remove unnecessary token usage when invoking devfile/library because we dont need a token to read byte strings
  • Adds controller tests out lined in https://issues.redhat.com/browse/DEVHAS-490

Which issue(s)/story(ies) does this PR fixes:

https://issues.redhat.com/browse/DEVHAS-490

PR acceptance criteria:

  • Unit/Functional tests

  • Documentation

  • Client Impact

How to test changes / Special notes to the reviewer:

Run the tests make test

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Nov 9, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved label Nov 9, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 13, 2023 •
edited
Loading

Codecov Report

Attention: 281 lines in your changes are missing coverage. Please review.

Comparison is base (5005e34) 83.98% compared to head (509e179) 81.12%.
Report is 26 commits behind head on main.

Files Patch % Lines
controllers/componentdetectionquery_controller.go 75.42% 45 Missing and 13 partials ⚠️
pkg/spi/spi.go 25.39% 47 Missing ⚠️
controllers/component_controller.go 56.25% 39 Missing and 3 partials ⚠️
cdq-analysis/pkg/componentdetectionquery.go 68.64% 33 Missing and 4 partials ⚠️
...pplicationsnapshotenvironmentbinding_controller.go 13.88% 30 Missing and 1 partial ⚠️
cdq-analysis/pkg/devfile.go 76.31% 20 Missing and 7 partials ⚠️
cdq-analysis/pkg/errors.go 22.22% 21 Missing ⚠️
cdq-analysis/pkg/util.go 76.92% 5 Missing and 4 partials ⚠️
cdq-analysis/pkg/detect.go 78.57% 6 Missing ⚠️
controllers/application_controller.go 75.00% 2 Missing ⚠️
... and 1 more
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #411      +/-   ##
==========================================
- Coverage   83.98%   81.12%   -2.86%     
==========================================
  Files          31       32       +1     
  Lines        4259     4753     +494     
==========================================
+ Hits         3577     3856     +279     
- Misses        514      707     +193     
- Partials      168      190      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maysunfaisal maysunfaisal marked this pull request as ready for review November 13, 2023 20:39
@openshift-ci openshift-ci bot requested a review from elsony November 13, 2023 20:39
@maysunfaisal maysunfaisal requested review from johnmcollier and removed request for elsony November 13, 2023 20:39
@maysunfaisal
Copy link
Member Author

/retest

2 similar comments
@maysunfaisal
Copy link
Member Author

/retest

@maysunfaisal
Copy link
Member Author

/retest

@maysunfaisal maysunfaisal changed the title Mocking CDQ Util for Private Repo Testing [DEVHAS-490] Mocking CDQ Util for Private Repo Testing Nov 13, 2023
@johnmcollier
Copy link
Member

/retest

1 similar comment
@johnmcollier
Copy link
Member

/retest

johnmcollier
johnmcollier reviewed Nov 14, 2023
View reviewed changes
Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

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

No major concerns from me. Just two suggestions

cdq-analysis/pkg/mock.go
@@ -0,0 +1,68 @@
/*
Copy link
Member

@johnmcollier johnmcollier Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can you add this to the codecov ignore list (you'll need to rebase to pull it in)?

cdq-analysis/pkg/componentdetectionquery.go Outdated
// return values are for testing purpose
func CloneAndAnalyze(k K8sInfoClient, namespace, name, context string, cdqInfo *CDQInfoClient) (map[string][]byte, map[string]string, map[string]string, map[string][]int, string, error) {
// CloneAndAnalyze clones and analyzes the Git repo with Alizer if necessary
func CloneAndAnalyze(k K8sInfoClient, namespace, name, context string, cdqInfo *CDQInfo, cdqUtil CDQUtil) (map[string][]byte, map[string]string, map[string]string, map[string][]int, string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I realize there wasn't already a function signature, but do you mind adding one? it's hard telling what the 5 other return values are for.

@maysunfaisal
Mocking CDQ Util 1 - mock valid token access
99cbfaa 
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
@maysunfaisal
Add controller test for CDQ
73922f3 
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
@maysunfaisal
Update test name typo and remove dup test
59d3338 
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
@maysunfaisal
Address PR feedback for func signature
509e179 
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
johnmcollier
johnmcollier approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

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

Looks fine from my end

@openshift-ci openshift-ci bot added the lgtm label Nov 14, 2023
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Nov 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnmcollier, maysunfaisal

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [johnmcollier,maysunfaisal]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@maysunfaisal maysunfaisal merged commit bf72a7c into redhat-appstudio:main Nov 14, 2023
7 of 9 checks passed
@maysunfaisal maysunfaisal mentioned this pull request Nov 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnmcollier johnmcollier johnmcollier approved these changes

@kim-tsao kim-tsao Awaiting requested review from kim-tsao

Assignees

@johnmcollier johnmcollier

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maysunfaisal @johnmcollier