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

ast cli #422

Merged
merged 10 commits into from
Sep 22, 2020
Merged

ast cli #422

merged 10 commits into from
Sep 22, 2020

Conversation

AvivCx
Copy link
Contributor

@AvivCx AvivCx commented Sep 21, 2020

Support AST scan in command line of CxFlow

Copy link
Contributor

@alex-ko-dev alex-ko-dev left a comment

Choose a reason for hiding this comment

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

Can we extract common logic from AstCliSteps and ScaCliSteps?



Scenario Outline: Testing break-build functionality
When running a AST scan with break-build on <issue-type>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be expanded a little:
running a AST scan with break-build enabled and command line arguments


Examples:
| issue-type | exit-code-number |
#| success | 0 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is success commented out?

Comment on lines 185 to 186
if (expectedIssueCount == AT_LEAST_ONE) {
Assert.assertTrue("Expected at least one issue in bug tracker.", actualIssueCount > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

The AT_LEAST_ONE case is not relevant for astCliScan.feature.


@Then("bug tracker contains {} issues")
public void validateBugTrackerIssues(int countIssues) {
int expectedIssueCount = countIssues;
Copy link
Contributor

Choose a reason for hiding this comment

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

countIssues argument can be renamed into expectedIssueCount, so we won't need an additional variable here.


private static Path getTempDir() {
String systemTempDir = FileUtils.getTempDirectoryPath();
String subdir = String.format("sca-cli-test-%s", UUID.randomUUID());
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 'ast-cli-test-%s'

Examples:
| scanner | issueNumber |
#| SCA | 5 |
| AST | 32 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Please be aware that tests with fixed issue counts may be fragile. I.e. issue count may change due to changes on the backend. I've already encountered it with SCA in astScanProcessing.feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will remove the SCA here

Copy link
Contributor

Choose a reason for hiding this comment

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

use a description of what is the meaning of the number instead of the actual number

And no exception is thrown
Examples:
| scanner | issueNumber |
#| SCA | 5 |
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the examples should either be ("AST", "AST,SCA") or just ("AST").
If the SCA line is not needed, please remove it.

public void setScanInitiator(String initiatorList) {
String[] intiators ;

if(!initiatorList.contains(SEPARATOR)){
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this hole if is equal to "intiators = initiatorList.split(SEPARATOR);"

Copy link
Contributor

Choose a reason for hiding this comment

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

when there's no separators, it will not work

Copy link
Contributor

Choose a reason for hiding this comment

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

if the separator is not found it returns an array of size 1 containing the original value

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed by "intiators = initiatorList.split(SEPARATOR);"

intiators = initiatorList.split(SEPARATOR);
}

List<String> scanners = new LinkedList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

List scanners = Arrays.stream(initiatorList.split(SEPARATOR))
.flatMap(intiators ->
Stream.of(ScaProperties.CONFIG_PREFIX , AstProperties.CONFIG_PREFIX)
.filter(intiators::equalsIgnoreCase))
.collect(Collectors.toList());
flowProperties.setEnabledVulnerabilityScanners(scanners);

Copy link
Contributor

Choose a reason for hiding this comment

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

or we can overload the flowProperties.setEnabledVulnerabilityScanners to get an ellipse of scanners, simplify the uses allover the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ofer pls don't make me change everything to Java 8

Copy link
Contributor

@ofersk ofersk Sep 22, 2020

Choose a reason for hiding this comment

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

then use retainAll (from List)
List x1 = new ArrayList(List.of("a","b","c"));
List x2 = List.of("a","b");
x1.retainAll(x2); //result is: x1 = ["a","b"]

} catch (Throwable e) {
exception = e;
}
cxFlowExecutionException = exception;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need the "exception" variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

because there's another step which is checking the exception class. In validateExitCode()

Copy link
Contributor

Choose a reason for hiding this comment

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

cxFlowExecutionException is all you need, the temporary "exception" lives for just 6 lines of code.
just use the cxFlowExecutionException in its place

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed

alex-ko-dev
alex-ko-dev previously approved these changes Sep 22, 2020
alex-ko-dev
alex-ko-dev previously approved these changes Sep 22, 2020
@sonarcloud
Copy link

sonarcloud bot commented Sep 22, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

warning The version of Java (1.8.0_265) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@olgakil olgakil merged commit 65ff1dc into develop Sep 22, 2020
@olgakil olgakil deleted the pr-orly-ast-cli branch September 23, 2020 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants