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

[JENKINS-54368] - User should be able to control the test execution failure #182

Merged
merged 21 commits into from
Nov 5, 2018

Conversation

aslaakso
Copy link

@aslaakso aslaakso commented Oct 26, 2018

@AppVeyorBot
Copy link


//add cleanup test
if(StringUtils.isEmpty(this.cleanupTest)){
props.put("CleanupTest1", "");
Copy link

Choose a reason for hiding this comment

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

Define a constant instead of duplicating this literal "CleanupTest1" 3 times.


//add number of reruns
if(StringUtils.isEmpty(this.numberOfReruns)){
props.put("Reruns1", "0");
Copy link

Choose a reason for hiding this comment

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

Define a constant instead of duplicating this literal "Reruns1" 3 times.

if (cleanupTests != null && cleanupTests.Count() > 0 && validCleanupTests.Count == 0)
{
ConsoleWriter.WriteLine(Resources.LauncherNoValidCleanupTests);
return null;
Copy link

Choose a reason for hiding this comment

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

Avoid too many return statements within this method.

* @return an mtbx file with tests, a single test or a list of tests from test folder
*/
public List<String> getBuildTests() {
List<String> buildTests = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

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

Remove this useless assignment to local variable "buildTests".


//run the tests!
RunTests(runner, resultsFilename);
} else {//TestStorageType.Alm
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

Copy link

@grount grount left a comment

Choose a reason for hiding this comment

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

  • Please open a JIRA issue with the PR.
  • Review codeclimate issues.
  • And address my comments
  • Your change in config.jelly of runFromFileBuilder breaks a test, see appveyor build.

@AppVeyorBot
Copy link

* @param folder the test path setup in the configuration (can be the an mtbx file, a single test or a folder containing other tests)
* @return a list of tests
*/
private List<String> listFilesForFolder(final File folder) {
Copy link

Choose a reason for hiding this comment

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

Method listFilesForFolder has a Cognitive Complexity of 14 (exceeds 5 allowed). Consider refactoring.

* @param rerunSettings the list of current tests
* @return the updated list of tests to rerun
*/
private List<String> getTests(List<String> buildTests, List<RerunSettings> rerunSettings){
Copy link

Choose a reason for hiding this comment

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

Method getTests has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@grount grount changed the title US - User should be able to control the test execution failure [JENKINS-54368] - User should be able to control the test execution failure Oct 31, 2018
@grount grount added this to the 5.5.3-beta milestone Oct 31, 2018
@@ -48,7 +49,11 @@

public final static EnumDescription FAST_RUN_MODE = new EnumDescription("Fast", "Fast");
public final static EnumDescription NORMAL_RUN_MODE = new EnumDescription("Normal", "Normal");
public final static EnumDescription ANY_BUILD_TEST = new EnumDescription("Of any of the build's tests", "Of any of the build's tests");
Copy link

Choose a reason for hiding this comment

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

You should create your model under uft folder and just introduce a filed inside your own model.

@@ -0,0 +1,93 @@
/*
Copy link

Choose a reason for hiding this comment

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

src/main/java/com/microfocus/application/automation/tools/uft/.../RerunSettings.java

@@ -286,6 +293,61 @@ public void setFsTests(String fsTests) {
runFromFileModel.setFsTests(fsTests);
}

public String getNumberOfReruns() { return runFromFileModel.getNumberOfReruns(); }
Copy link

Choose a reason for hiding this comment

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

When you will introduce your own model that will implement abstractDescriableImpl there won't be a need in exposing the field getters/setters.

@@ -661,6 +786,7 @@ public Properties getProperties() {

private Properties createProperties(EnvVars envVars) {
Properties props = new Properties();
List<String> buildTestsList = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

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

Remove this useless assignment to local variable "buildTestsList".

* @param rerunSettingModels the list of current tests
* @return the updated list of tests to rerun
*//*
private List<String> getTests(List<String> buildTests, List<RerunSettingsModel> rerunSettingModels){
Copy link

Choose a reason for hiding this comment

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

This block of commented-out lines of code should be removed.

this.fsTestType = fsTestType;
}

/*public List<RerunSettingsModel> getRerunSettingModels() {
Copy link

Choose a reason for hiding this comment

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

This block of commented-out lines of code should be removed.

* @return an mtbx file with tests, a single test or a list of tests from test folder
*/
public List<String> getBuildTests() {
//String directory = "C:\\Users\\laakso\\Documents\\UFT_tests";
Copy link

Choose a reason for hiding this comment

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

This block of commented-out lines of code should be removed.

@@ -102,6 +103,11 @@ public RunFromFileBuilder(String fsTests) {
runFromFileModel = new RunFromFileSystemModel(fsTests);
}

/*public RunFromFileBuilder(String fsTests, UftSettingsModel uftSettingsModel) {
Copy link

Choose a reason for hiding this comment

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

This block of commented-out lines of code should be removed.

@AppVeyorBot
Copy link


public List<EnumDescription> getFsTestTypes() { return fsTestTypes; }

public void addToProperties(Properties props){
Copy link

Choose a reason for hiding this comment

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

Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed.

* @return an mtbx file with tests, a single test or a list of tests from test folder
*/
public static List<String> getBuildTests(String fsTestPath) {
// String directory = this.fsTestPath;
Copy link

Choose a reason for hiding this comment

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

This block of commented-out lines of code should be removed.

return buildTests;
}

return null;
Copy link

Choose a reason for hiding this comment

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

Return an empty collection instead of null.

*/
public static List<String> getTests(List<String> buildTests, List<RerunSettingsModel> rerunSettingModels){
if(buildTests == null || rerunSettingModels == null){
return null;
Copy link

Choose a reason for hiding this comment

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

Return an empty collection instead of null.

*/
public List<RerunSettingsModel> getRerunSettingsModels(){
List<String> testPaths = UftToolUtils.getTests(UftToolUtils.getBuildTests(fsTestPath), rerunSettingsModels);
for(String testPath : testPaths){
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 3 locations. Consider refactoring.

@AppVeyorBot
Copy link

if(testPaths != null) {
for (String testPath : testPaths) {
if (!UftToolUtils.listContainsTest(rerunSettings, testPath)) {
rerunSettings.add(new RerunSettingsModel(testPath, false, 0, ""));
Copy link

Choose a reason for hiding this comment

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

A "NullPointerException" could be thrown; "rerunSettings" is nullable here.

*/
public static List<RerunSettingsModel> getSettings(String fsTestPath, List<RerunSettingsModel> rerunSettings){
List<String> testPaths = getTests(getBuildTests(fsTestPath), rerunSettings);
if(testPaths != null) {
Copy link

Choose a reason for hiding this comment

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

Remove this expression which always evaluates to "true"

* @param rerunSettingModels the list of current tests
* @return the updated list of tests to rerun
*/
public static List<String> getTests(List<String> buildTests, List<RerunSettingsModel> rerunSettingModels){
Copy link

Choose a reason for hiding this comment

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

Method getTests has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.

* @param folder the test path setup in the configuration (can be the an mtbx file, a single test or a folder containing other tests)
* @return a list of tests
*/
public static List<String> listFilesForFolder(final File folder) {
Copy link

Choose a reason for hiding this comment

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

Method listFilesForFolder has a Cognitive Complexity of 14 (exceeds 5 allowed). Consider refactoring.

* @param rerunSettings the rerun settings models list to initialize
* @return the updated rerun settings list
*/
public static List<RerunSettingsModel> getSettings(String fsTestPath, List<RerunSettingsModel> rerunSettings){
Copy link

Choose a reason for hiding this comment

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

Method getSettings has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

@AppVeyorBot
Copy link

*
* @param props
*/
public void addToProperties(Properties props){
Copy link

Choose a reason for hiding this comment

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

Method addToProperties has a Cognitive Complexity of 21 (exceeds 5 allowed). Consider refactoring.

* @param folder the test path setup in the configuration (can be the an mtbx file, a single test or a folder containing other tests)
* @return a list of tests
*/
public static List<String> listFilesForFolder(final File folder) {
Copy link

Choose a reason for hiding this comment

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

Method listFilesForFolder has a Cognitive Complexity of 13 (exceeds 5 allowed). Consider refactoring.

@AppVeyorBot
Copy link

List<String> testPaths = UftToolUtils.getTests(UftToolUtils.getBuildTests(fsTestPath), rerunSettingsModels);
for(String testPath : testPaths){
if(!UftToolUtils.listContainsTest(rerunSettingsModels, testPath)) {
rerunSettingsModels.add(new RerunSettingsModel(testPath, false, 0, ""));
Copy link

Choose a reason for hiding this comment

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

A "NullPointerException" could be thrown; "rerunSettingsModels" is nullable here.

* @param folder the test path setup in the configuration (can be the an mtbx file, a single test or a folder containing other tests)
* @return a list of tests
*/
public static List<String> listFilesForFolder(final File folder) {
Copy link

Choose a reason for hiding this comment

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

Method listFilesForFolder has a Cognitive Complexity of 13 (exceeds 5 allowed). Consider refactoring.

*
* @param props
*/
public void addToProperties(Properties props){
Copy link

Choose a reason for hiding this comment

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

Method addToProperties has a Cognitive Complexity of 15 (exceeds 5 allowed). Consider refactoring.

@AppVeyorBot
Copy link

@grount
Copy link

grount commented Nov 4, 2018

Approved for test build

@AppVeyorBot
Copy link

@@ -0,0 +1,152 @@
/*
Copy link

@grount grount Nov 4, 2018

Choose a reason for hiding this comment

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

Please move this method to uft package.

Copy link

@grount grount left a comment

Choose a reason for hiding this comment

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

From now on, all new classes that would be created by uft should be in uft package.

return buildTests;
}

return null;
Copy link

Choose a reason for hiding this comment

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

Return an empty collection instead of null.

List<String> testPaths = UftToolUtils.getTests(UftToolUtils.getBuildTests(fsTestPath), rerunSettingsModels);
for(String testPath : testPaths){
if(!UftToolUtils.listContainsTest(rerunSettingsModels, testPath)) {
rerunSettingsModels.add(new RerunSettingsModel(testPath, false, 0, ""));
Copy link

Choose a reason for hiding this comment

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

A "NullPointerException" could be thrown; "rerunSettingsModels" is nullable here.

*
* @param props
*/
public void addToProperties(Properties props){
Copy link

Choose a reason for hiding this comment

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

Method addToProperties has a Cognitive Complexity of 15 (exceeds 5 allowed). Consider refactoring.

* @param rerunSettingModels the list of current tests
* @return the updated list of tests to rerun
*/
public static List<String> getTests(List<String> buildTests, List<RerunSettingsModel> rerunSettingModels){
Copy link

Choose a reason for hiding this comment

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

Method getTests has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.

* @param folder the test path setup in the configuration (can be the an mtbx file, a single test or a folder containing other tests)
* @return a list of tests
*/
public static List<String> listFilesForFolder(final File folder) {
Copy link

Choose a reason for hiding this comment

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

Method listFilesForFolder has a Cognitive Complexity of 13 (exceeds 5 allowed). Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Nov 5, 2018

Code Climate has analyzed commit 9a3972c and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3
Bug Risk 1
Clarity 1

View more on Code Climate.

@grount grount removed the needs-fix label Nov 5, 2018
@grount
Copy link

grount commented Nov 5, 2018

Approved for test build

@AppVeyorBot
Copy link

@grount grount removed the needs-build label Nov 5, 2018
@grount grount merged commit 9cf0a44 into jenkinsci:latest Nov 5, 2018
grount pushed a commit that referenced this pull request Mar 7, 2019
* defect #803014: octane-sonar integration works only with "maven project" jobs

* tech: minor fixes for coverage classes
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.

3 participants