Merge pull request #119 from adangel:update-manual-integration-test

Update manual integration test for PMD 7 #119

Author: adangel

History.md

@@ -4,6 +4,7 @@
 
 ## Enhancements
 * [#118](https://github.com/pmd/pmd-regression-tester/pull/118): Update js libraries
+* [#119](https://github.com/pmd/pmd-regression-tester/pull/119): Update manual integration test for PMD 7
 * [#120](https://github.com/pmd/pmd-regression-tester/pull/120): Support new PMD 7 binary dist filename
 
 ## Fixed Issues

README.rdoc

@@ -136,10 +136,10 @@ The tool creates the following folders:
   bundle exec pmdtester ... # run this to directly execute pmdtester from source
 
   Run a single test class, e.g.:
-  bundle exec ruby -I test test/test_diff_report_builder.rb
+  bundle exec ruby -I test test/test_project_diff_report.rb
 
   Run a single test, e.g.:
-  bundle exec ruby -I test test/test_diff_report_builder.rb -n test_diff_report_builder
+  bundle exec ruby -I test test/test_project_diff_report.rb -n test_diff_report_builder
 
 === Releasing
 

lib/pmdtester/builders/pmd_report_builder.rb

@@ -60,22 +60,12 @@ def build_pmd(into_dir:)
         # In CI, that's not a problem, because the workspace is always fresh.
         logger.warn "#{@pmd_branch_name}: Reusing already existing #{pmd_dist_target}"
       else
-        logger.info "#{@pmd_branch_name}: Building PMD #{@pmd_version}..."
-        package_cmd = './mvnw clean package' \
-                      ' -Dmaven.test.skip=true' \
-                      ' -Dmaven.javadoc.skip=true' \
-                      ' -Dmaven.source.skip=true' \
-                      ' -Dcheckstyle.skip=true' \
-                      ' -Dpmd.skip=true' \
-                      ' -T1C -B'
-        Cmd.execute_successfully(package_cmd)
-
+        build_pmd_with_maven
         pmd_dist_target, binary_exists = find_pmd_dist_target
         unless binary_exists
           logger.error "#{@pmd_branch_name}: Dist zip not found at #{pmd_dist_target}!"
           raise "No Dist zip found at #{pmd_dist_target}"
         end
-
       end
 
       logger.info "#{@pmd_branch_name}: Extracting the zip"
@@ -101,7 +91,7 @@ def get_last_commit_message
 
     def generate_pmd_report(project)
       error_recovery_options = @error_recovery ? 'PMD_JAVA_OPTS="-Dpmd.error_recovery -ea" ' : ''
-      fail_on_violation = should_use_long_cli_options? ? '--fail-on-violation false' : '-failOnViolation false'
+      fail_on_violation = create_failonviolation_option
       auxclasspath_option = create_auxclasspath_option(project)
       pmd_cmd = "#{error_recovery_options}" \
                 "#{determine_run_path} -d #{project.local_source_path} -f xml " \
@@ -218,6 +208,16 @@ def create_auxclasspath_option(project)
       auxclasspath_option
     end
 
+    def create_failonviolation_option
+      if pmd7?
+        '--no-fail-on-violation'
+      elsif should_use_long_cli_options?
+        '--fail-on-violation false'
+      else
+        '-failOnViolation false'
+      end
+    end
+
     def pmd7?
       Semver.compare(@pmd_version, '7.0.0-SNAPSHOT') >= 0
     end
@@ -244,5 +244,18 @@ def find_pmd_dist_target
       end
       [pmd_dist_target, binary_exists]
     end
+
+    def build_pmd_with_maven
+      logger.info "#{@pmd_branch_name}: Building PMD #{@pmd_version}..."
+      package_cmd = './mvnw clean package' \
+                    ' -Dmaven.test.skip=true' \
+                    ' -Dmaven.javadoc.skip=true' \
+                    ' -Dmaven.source.skip=true' \
+                    ' -Dcheckstyle.skip=true' \
+                    ' -Dpmd.skip=true' \
+                    ' -T1C -B'
+      logger.debug "#{@pmd_branch_name}: maven command: #{package_cmd}"
+      Cmd.execute_successfully(package_cmd)
+    end
   end
 end

lib/pmdtester/cmd.rb

@@ -30,6 +30,7 @@ def self.execute_successfully(cmd)
       stdout, stderr, status = internal_execute(cmd)
 
       unless status.success?
+        logger.error "Command failed: #{cmd}"
         logger.error stdout
         logger.error stderr
         raise CmdException.new(cmd, stdout, stderr, status)

pmdtester.gemspec

@@ -11,7 +11,7 @@ Gem::Specification.new do |s|
   s.metadata = { "bug_tracker_uri" => "https://github.com/pmd/pmd-regression-tester/issues", "homepage_uri" => "https://pmd.github.io", "source_code_uri" => "https://github.com/pmd/pmd-regression-tester" } if s.respond_to? :metadata=
   s.require_paths = ["lib".freeze]
   s.authors = ["Andreas Dangel".freeze, "Binguo Bao".freeze, "Cl\u00E9ment Fournier".freeze]
-  s.date = "2023-05-26"
+  s.date = "2023-05-27"
   s.description = "A regression testing tool ensure that new problems and unexpected behaviors will not be introduced to PMD project after fixing an issue , and new rules can work as expected.".freeze
   s.email = ["[email protected]".freeze, "[email protected]".freeze, "[email protected]".freeze]
   s.executables = ["pmdtester".freeze]

test/integration_test_pmd_report_builder.rb

@@ -10,14 +10,21 @@ def setup
     `rake clean`
   end
 
-  def test_build
+  # Tests whether we can build successfully PMD from the sources of the master branch.
+  # The master branch should always be buildable by the regression tester. For older
+  # versions, we can rely on baselines.
+  #
+  # Note 1: Although a base branch is configured here, this is not used. Only the patch
+  # branch is built.
+  # Note 2: We use a limited set of projects and rules, to make the test faster.
+  def test_build_master_branch
     argv = ['-r', 'target/repositories/pmd',
-            '-b', 'master',
-            '-p', 'origin/pmd/7.0.x',
+            '-b', 'pmd_releases/6.55.0',
+            '-p', 'master',
             '-c', 'test/resources/integration_test_pmd_report_builder/pmd7-config.xml',
             '-l', 'test/resources/integration_test_pmd_report_builder/project-test.xml',
             '--error-recovery',
-            '--debug',
+            # '--debug',
             '--threads', Etc.nprocessors.to_s]
 
     options = PmdTester::Options.new(argv)
@@ -27,6 +34,6 @@ def test_build
     builder.build
 
     assert_equal(0, $CHILD_STATUS.exitstatus)
-    assert_path_exist('target/reports/origin_pmd_7.0.x/checkstyle/pmd_report.xml')
+    assert_path_exist('target/reports/master/checkstyle/pmd_report.xml')
   end
 end

test/integration_test_runner.rb

@@ -69,22 +69,23 @@ def test_single_mode_with_html_flag_option
   end
 
   def test_online_mode
-    # This test depends on the file test_branch_2-baseline.zip being available at:
-    # https://pmd-code.org/pmd-regression-tester/test_branch_2-baseline.zip
-    base_branch = 'test_branch_2'
-    argv = "-r target/repositories/pmd -m online -b #{base_branch} -p pmd_releases/6.41.0 " \
+    # This test depends on the file pmd_releases_7.0.0-rc1-baseline.zip being available at:
+    # https://pmd-code.org/pmd-regression-tester/pmd_releases_7.0.0-rc1-baseline.zip
+    base_branch = 'pmd_releases/7.0.0-rc1'
+    patch_branch = 'pmd_releases/7.0.0-rc2'
+    argv = "-r target/repositories/pmd -m online -b #{base_branch} -p #{patch_branch} " \
         '--baseline-download-url https://pmd-code.org/pmd-regression-tester/' \
         ' --threads ' + Etc.nprocessors.to_s
 
     system("bundle exec bin/pmdtester #{argv}")
 
-    assert_path_exist("target/reports/#{base_branch}-baseline.zip")
-    assert_path_exist("target/reports/#{base_branch}/checkstyle/pmd_report.xml")
-    assert_path_exist("target/reports/#{base_branch}/spring-framework/pmd_report.xml")
-    assert_path_exist('target/reports/pmd_releases_6.41.0/checkstyle/pmd_report.xml')
-    assert_path_exist('target/reports/pmd_releases_6.41.0/checkstyle/config.xml')
-    assert_path_exist('target/reports/pmd_releases_6.41.0/spring-framework/pmd_report.xml')
-    assert_path_exist('target/reports/pmd_releases_6.41.0/spring-framework/config.xml')
+    assert_path_exist("target/reports/#{base_branch.tr('/', '_')}-baseline.zip")
+    assert_path_exist("target/reports/#{base_branch.tr('/', '_')}/checkstyle/pmd_report.xml")
+    assert_path_exist("target/reports/#{base_branch.tr('/', '_')}/spring-framework/pmd_report.xml")
+    assert_path_exist("target/reports/#{patch_branch.tr('/', '_')}/checkstyle/pmd_report.xml")
+    assert_path_exist("target/reports/#{patch_branch.tr('/', '_')}/checkstyle/config.xml")
+    assert_path_exist("target/reports/#{patch_branch.tr('/', '_')}/spring-framework/pmd_report.xml")
+    assert_path_exist("target/reports/#{patch_branch.tr('/', '_')}/spring-framework/config.xml")
     assert_path_exist('target/reports/diff/checkstyle/index.html')
     assert_path_exist('target/reports/diff/spring-framework/index.html')
     assert_path_exist('target/reports/diff/index.html')

test/manual_integration_tests.rb

@@ -46,29 +46,34 @@ def test_case_1_single_java_rule_changed
     assert_equal(0, @summary[:violations][:changed], 'found changed violations')
     assert_equal(0, @summary[:violations][:new], 'found new violations')
     # These are the artificially created false-negatives for AbstractClassWithoutAbstractMethod rule
-    # checkstyle: 204 violations
+    # checkstyle: 195 violations
     # spring-framework: 280 violations
     # openjdk11: 29 violations
-    # -> total = 513
-    assert_equal(204 + 280 + 29, @summary[:violations][:removed], 'found removed violations')
+    # -> total = 504
+    assert_equal(195 + 280 + 29, @summary[:violations][:removed], 'found removed violations')
 
     # errors might have been caused in the baseline for other rules (only visible in the stacktrace)
     # hence they might appear as removed
 
-    # project "apex-link" has 2 errors, since we only executed java rules, 2 errors are removed
-    assert_equal(2, @summary[:errors][:removed], 'found removed errors')
-    assert_equal(0, @summary[:errors][:changed], 'found changed errors')
+    # project "apex-link" has 2 errors removed, since we only executed java rules
+    # project "checkstyle" has 10 errors removed and 1 changed
+    # project "openjdk-11" has 0 error removed or changed
+    # project "spring-framework" has 1 error removed
+    # each project has 1 config error removed (LoosePackageCoupling dysfunctional): in total 7 config errors removed
+    assert_equal(13, @summary[:errors][:removed], 'found removed errors')
+    # The stack overflow exception might vary in the beginning/end of the stack frames shown
+    # This stack overflow error is from checkstyle's InputIndentationLongConcatenatedString.java
+    # instead of assert_equal(0, @summary[:errors][:changed], 'found changed errors')
+    # allow 0 or 1 changed errors
+    assert @summary[:errors][:changed] <= 1
     assert_equal(0, @summary[:errors][:new], 'found new errors')
     assert_equal(0, @summary[:configerrors][:changed], 'found changed configerrors')
     assert_equal(0, @summary[:configerrors][:new], 'found new configerrors')
-    # Only the rule AbstractClassWithoutAbtractMethod has been executed, so the
-    # configerrors about LoosePackageCoupling are gone, one for each project
-    # we now have 7 projects in total (4 java, 3 apex)
     assert_equal(7, @summary[:configerrors][:removed], 'found removed configerrors')
 
     assert_equal("This changeset changes 0 violations,\n" \
                  "introduces 0 new violations, 0 new errors and 0 new configuration errors,\n" \
-                 'removes 513 violations, 2 errors and 7 configuration errors.',
+                 'removes 504 violations, 13 errors and 7 configuration errors.',
                  create_summary_message)
 
     assert_file_equals("#{PATCHES_PATH}/expected_patch_config_1.xml", 'target/reports/diff/patch_config.xml')
@@ -93,20 +98,25 @@ def test_case_2_single_xpath_rule_changed
     # errors might have been caused in the baseline for other rules (only visible in the stacktrace)
     # hence they might appear as removed
 
-    # project "apex-link" has 2 errors, since we only executed java rules, 2 errors are removed
-    assert_equal(2, @summary[:errors][:removed], 'found removed errors')
-    assert_equal(0, @summary[:errors][:changed], 'found changed errors')
+    # project "apex-link" has 2 errors removed, since we only executed java rules
+    # project "checkstyle" has 5 errors removed and 1 errors changed
+    # project "openjdk-11" has 0 error removed or changed
+    # project "spring-framework" has 1 error removed
+    # each project has 1 config error removed (LoosePackageCoupling dysfunctional): in total 7 config errors removed
+    assert_equal(8, @summary[:errors][:removed], 'found removed errors')
+    # The stack overflow exception might vary in the beginning/end of the stack frames shown
+    # This stack overflow error is from checkstyle's InputIndentationLongConcatenatedString.java
+    # instead of assert_equal(0, @summary[:errors][:changed], 'found changed errors')
+    # allow 0 or 1 changed errors
+    assert @summary[:errors][:changed] <= 1
     assert_equal(0, @summary[:errors][:new], 'found new errors')
     assert_equal(0, @summary[:configerrors][:changed], 'found changed configerrors')
     assert_equal(0, @summary[:configerrors][:new], 'found new configerrors')
-    # Only the rule AvoidMessageDigestField and all other rules from bestpractices have been executed, so the
-    # configerrors about LoosePackageCoupling are gone, one for each project
-    # we now have 7 projects in total (4 java, 3 apex)
     assert_equal(7, @summary[:configerrors][:removed], 'found removed configerrors')
 
     assert_equal("This changeset changes 0 violations,\n" \
                  "introduces 0 new violations, 0 new errors and 0 new configuration errors,\n" \
-                 'removes 22 violations, 2 errors and 7 configuration errors.',
+                 'removes 22 violations, 8 errors and 7 configuration errors.',
                  create_summary_message)
 
     assert_file_equals("#{PATCHES_PATH}/expected_patch_config_2.xml", 'target/reports/diff/patch_config.xml')
@@ -115,7 +125,7 @@ def test_case_2_single_xpath_rule_changed
 
   def test_case_3_change_in_core
     checkout_pmd_branch
-    prepare_patch_branch('patch_test_case_3_modify_PMD.patch', 'test-case-3')
+    prepare_patch_branch('patch_test_case_3_modify_pmd-core.patch', 'test-case-3')
 
     run_pmd_tester
 
@@ -125,7 +135,11 @@ def test_case_3_change_in_core
     assert_equal(0, @summary[:violations][:new], 'found new violations')
     assert_equal(0, @summary[:violations][:removed], 'found removed violations')
     assert_equal(0, @summary[:errors][:removed], 'found removed errors')
-    assert_equal(0, @summary[:errors][:changed], 'found changed errors')
+    # The stack overflow exception might vary in the beginning/end of the stack frames shown
+    # This stack overflow error is from checkstyle's InputIndentationLongConcatenatedString.java
+    # instead of assert_equal(0, @summary[:errors][:changed], 'found changed errors')
+    # allow 0 or 1 changed errors
+    assert @summary[:errors][:changed] <= 1
     assert_equal(0, @summary[:errors][:new], 'found new errors')
     assert_equal(0, @summary[:configerrors][:changed], 'found changed configerrors')
     assert_equal(0, @summary[:configerrors][:new], 'found new configerrors')
@@ -231,7 +245,7 @@ def checkout_pmd_branch(branch = 'master')
       system('git config user.email "[email protected]"')
       system('git config user.name "PMD CI (pmd-bot)"')
       # remove any already existing binary to force a rebuild
-      FileUtils.rm Dir.glob('pmd-dist/target/pmd-bin-*.zip')
+      FileUtils.rm Dir.glob('pmd-dist/target/pmd-*.zip')
     end
   end
 
@@ -248,19 +262,19 @@ def prepare_patch_branch(patch_file, local_branch, base_branch = 'master')
   end
 
   def assert_master_baseline
-    assert_path_exist('target/reports/master/checkstyle/config.xml')
-    assert_path_exist('target/reports/master/checkstyle/report_info.json')
-    assert_path_exist('target/reports/master/checkstyle/pmd_report.xml')
-    assert(File.size('target/reports/master/checkstyle/pmd_report.xml') > 50 * 1024 * 1024)
-
-    assert_path_exist('target/reports/master/openjdk-11/config.xml')
-    assert_path_exist('target/reports/master/openjdk-11/report_info.json')
-    assert_path_exist('target/reports/master/openjdk-11/pmd_report.xml')
-    assert(File.size('target/reports/master/openjdk-11/pmd_report.xml') > 100 * 1024 * 1024)
-
-    assert_path_exist('target/reports/master/spring-framework/config.xml')
-    assert_path_exist('target/reports/master/spring-framework/report_info.json')
-    assert_path_exist('target/reports/master/spring-framework/pmd_report.xml')
-    assert(File.size('target/reports/master/spring-framework/pmd_report.xml') > 150 * 1024 * 1024)
+    assert_master_baseline_project('checkstyle', 40 * 1024 * 1024)
+    assert_master_baseline_project('openjdk-11', 80 * 1024 * 1024)
+    assert_master_baseline_project('spring-framework', 100 * 1024 * 1024)
+    assert_master_baseline_project('java-regression-tests', 100 * 1024)
+    assert_master_baseline_project('apex-link', 10 * 1024)
+    assert_master_baseline_project('fflib-apex-common', 400 * 1024)
+    assert_master_baseline_project('Schedul-o-matic-9000', 20 * 1024)
+  end
+
+  def assert_master_baseline_project(project_name, report_size_in_bytes)
+    assert_path_exist("target/reports/master/#{project_name}/config.xml")
+    assert_path_exist("target/reports/master/#{project_name}/report_info.json")
+    assert_path_exist("target/reports/master/#{project_name}/pmd_report.xml")
+    assert(File.size("target/reports/master/#{project_name}/pmd_report.xml") > report_size_in_bytes)
   end
 end

test/resources/manual_integration_tests/patch_test_case_1_disable_AbstractClassWithoutAbstractMethod.patch

@@ -1,7 +1,7 @@
-From df7e43732794442b7c787493a2189b0c4a0e3f85 Mon Sep 17 00:00:00 2001
+From ab94c0fed1813eb5e8376be51a7c93164652e26b Mon Sep 17 00:00:00 2001
 From: Andreas Dangel <[email protected]>
-Date: Thu, 14 Jan 2021 11:25:58 +0100
-Subject: [PATCH] pmd-regression-test: test case 1 - single java rule changed
+Date: Thu, 4 May 2023 19:44:31 +0200
+Subject: [PATCH] test case 1 - single java rule changed
 
 A single rule (java class) is changed. Only this rule should be executed
 and only this rule should be compared (ruleset is filtered).
@@ -15,34 +15,33 @@ exactly this rule.
  2 files changed, 4 insertions(+), 1 deletion(-)
 
 diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AbstractClassWithoutAbstractMethodRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AbstractClassWithoutAbstractMethodRule.java
-index 3f20b559d5..4ef489c23b 100644
+index 0d0d8c33e4..972e1bd62a 100644
 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AbstractClassWithoutAbstractMethodRule.java
 +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AbstractClassWithoutAbstractMethodRule.java
-@@ -35,7 +35,7 @@ public class AbstractClassWithoutAbstractMethodRule extends AbstractJavaRule {
-             }
+@@ -22,7 +22,7 @@ public class AbstractClassWithoutAbstractMethodRule extends AbstractJavaRulechai
          }
-         if (countOfAbstractMethods == 0) {
+ 
+         if (node.getDeclarations(ASTMethodDeclaration.class).none(ASTMethodDeclaration::isAbstract)) {
 -            addViolation(data, node);
 +            //addViolation(data, node);
          }
          return data;
      }
 diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AbstractClassWithoutAbstractMethodTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AbstractClassWithoutAbstractMethodTest.java
-index a7ff179f29..ac4d852e26 100644
+index b319c5e9f1..77698edb60 100644
 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AbstractClassWithoutAbstractMethodTest.java
 +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/AbstractClassWithoutAbstractMethodTest.java
 @@ -4,8 +4,11 @@
 
  package net.sourceforge.pmd.lang.java.rule.bestpractices;
 
-+import org.junit.Ignore;
++import org.junit.jupiter.api.Disabled;
 +
  import net.sourceforge.pmd.testframework.PmdRuleTst;
 
-+@Ignore
- public class AbstractClassWithoutAbstractMethodTest extends PmdRuleTst {
++@Disabled
+ class AbstractClassWithoutAbstractMethodTest extends PmdRuleTst {
      // no additional unit tests
  }
 -- 
-2.29.2
-
+2.39.2