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

[RFR] Update dependency check #221

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

Conversation

nachandr
Copy link
Collaborator

@nachandr nachandr commented Nov 19, 2024

The following scenario is currently not checked . This PR addresses the missing scenario -
When dependencies are not specified in the test and the API request response returns dependencies .

Before fix,
When dependencies are not specified in the test and the API request response returns dependencies , the test passed.

After fix,
When dependencies are not specified in the test and the API request response returns dependencies , the test fails.

Note: An analysis test will still pass when dependencies are not specified in the test and if the API response request returns an empty list for dependencies. The fix doesn't change this behavior.

@nachandr nachandr requested review from aufi and jmle November 19, 2024 17:14
@nachandr nachandr changed the title [RFR] Update dependency check [W] Update dependency check Nov 19, 2024
@nachandr nachandr changed the title [W] Update dependency check [WIP] Update dependency check Nov 19, 2024
@nachandr nachandr force-pushed the update_dependency_check branch 2 times, most recently from 408232a to 7c149f8 Compare November 19, 2024 17:56
@nachandr nachandr changed the title [WIP] Update dependency check [RFR] Update dependency check Nov 19, 2024
@aufi
Copy link
Member

aufi commented Nov 20, 2024

IIUC the aim of this PR is to not allow pass analysis test without specifying dependencies.

image

I don't think this is something we want. Optional skip of Dependencies or Tags in test assertion (defined by the test case), looks as the right option to me.

For visiblity @mguetta1 @jmle

Signed-off-by: Nandini Chandra <[email protected]>
Signed-off-by: Nandini Chandra <[email protected]>
@mguetta1
Copy link
Collaborator

IIUC the aim of this PR is to not allow pass analysis test without specifying dependencies.

image

I don't think this is something we want. Optional skip of Dependencies or Tags in test assertion (defined by the test case), looks as the right option to me.

For visiblity @mguetta1 @jmle

I saw the need for such change after that Igor reported this regression bug: https://issues.redhat.com/browse/MTA-4169
At that time I wondered if we were seeing it in the API tests as well and realized that deps check was skipped for this app

@nachandr
Copy link
Collaborator Author

nachandr commented Nov 21, 2024

IIUC the aim of this PR is to not allow pass analysis test without specifying dependencies.

image

I don't think this is something we want. Optional skip of Dependencies or Tags in test assertion (defined by the test case), looks as the right option to me.

For visiblity @mguetta1 @jmle

Hi @aufi , Thanks for reviewing the PR. I noticed that when dependencies(or tags) are not specified for a test and when the API request response returns dependencies (or tags), the test still passes . The code changes address this scenario.

I'd like to clarify that an analysis test will still pass when dependencies (or tags) are not specified in the test iff the API response request returns the dependency/tag count as zero.
For eg: The tier 2 run for this PR shows that the Apache Wicket analysis passed (This test doesn't have dependencies listed)
Update_dependency

@aufi
Copy link
Member

aufi commented Nov 21, 2024

Understood, the question is if we need more assert empty dependencies list or ability to not check dependencies (in this "easy" way), deffering to others. @mguetta1 @sshveta

t.Errorf("\nDifferent dependency error. Got %+v\nExpected %+v.\n\n", got, expected)
}
// Check dependencies
if (len(gotAnalysis.Dependencies) == 0 && len(tc.Analysis.Dependencies) == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (len(gotAnalysis.Dependencies) == 0 && len(tc.Analysis.Dependencies) == 0) {
if tc.Analysis.Dependencies != nil {

@mguetta1
Copy link
Collaborator

Understood, the question is if we need more assert empty dependencies list or ability to not check dependencies (in this "easy" way), deffering to others. @mguetta1 @sshveta

To make it possible not to check dependencies in one case and check an expected empty list in other case, please see my suggestion above

@mguetta1 mguetta1 added cherry-pick/release-0.6 This PR should be cherry-picked to release-0.6 branch cherry-pick/release-0.5 This PR should be cherry-picked to release-0.5 branch. labels Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/release-0.5 This PR should be cherry-picked to release-0.5 branch. cherry-pick/release-0.6 This PR should be cherry-picked to release-0.6 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants