diff --git a/.github/workflows/conformance-report.yml b/.github/workflows/conformance-report.yml index d9dbbf3cee..bdcf83d8f9 100644 --- a/.github/workflows/conformance-report.yml +++ b/.github/workflows/conformance-report.yml @@ -5,6 +5,8 @@ env: PATH_TO_TEST_RUNNER: test/partiql-tests-runner CONFORMANCE_REPORT_RELATIVE_PATH: build/conformance-test-report COMPARISON_REPORT_NAME: comparison_report.md + COMPARISON_REPORT_NAME_WITH_LIMIT: comparison_report_limited.md + COMMENT_SIZE_LIMIT: 10 jobs: conformance-report: @@ -28,7 +30,7 @@ jobs: # Run the conformance tests and save to an Ion file. - name: gradle test of the conformance tests (can fail) and save to Ion file continue-on-error: true - run: gradle :test:partiql-tests-runner:ConformanceTestReport + run: gradle :test:partiql-tests-runner:generateTestReport # Upload conformance report for future viewing and comparison with future runs. - name: Upload `conformance-test-report` folder uses: actions/upload-artifact@v3 @@ -87,7 +89,7 @@ jobs: continue-on-error: true run: | cd ${{ github.event.pull_request.base.sha }} - gradle :test:partiql-tests-runner:ConformanceTestReport + gradle :test:partiql-tests-runner:generateTestReport - name: (If download of target branch conformance report fails) Move conformance test report of target branch to ./artifact directory if: ${{ steps.download-report.outcome == 'failure' }} continue-on-error: true @@ -95,7 +97,7 @@ jobs: mkdir -p $GITHUB_WORKSPACE/artifact cp -r $GITHUB_WORKSPACE/${{ github.event.pull_request.base.sha }}/$PATH_TO_TEST_RUNNER/$CONFORMANCE_REPORT_RELATIVE_PATH $GITHUB_WORKSPACE/artifact/$CONFORMANCE_REPORT_RELATIVE_PATH # Run conformance report comparison. Generates `comparison_report.md` - - name: Run conformance report comparison. Generates `comparison_report.md` + - name: Run conformance report comparison for artifact. Generates `comparison_report.md` continue-on-error: true run: | ARGS="$GITHUB_WORKSPACE/artifact $CONFORMANCE_REPORT_RELATIVE_PATH ${{ github.event.pull_request.base.sha }} $GITHUB_SHA $COMPARISON_REPORT_NAME" @@ -103,7 +105,18 @@ jobs: # Print conformance report to GitHub actions workflow summary page - name: Print markdown in run continue-on-error: true - run: echo $PATH_TO_TEST_RUNNER/$COMPARISON_REPORT_NAME >> $GITHUB_STEP_SUMMARY + run: cat $PATH_TO_TEST_RUNNER/$COMPARISON_REPORT_NAME >> $GITHUB_STEP_SUMMARY + # Upload the full comparison report to CI artifact + - name: Upload `comparison_report.md` + uses: actions/upload-artifact@v3 + with: + path: ${{ env.PATH_TO_TEST_RUNNER }}/comparison_report.md + # Rebuild the test report with a size limit for comment + - name: Run conformance report comparison for comment. Generates `comparison_report_limited.md` + continue-on-error: true + run: | + ARGS="$GITHUB_WORKSPACE/artifact $CONFORMANCE_REPORT_RELATIVE_PATH ${{ github.event.pull_request.base.sha }} $GITHUB_SHA $COMPARISON_REPORT_NAME_WITH_LIMIT $COMMENT_SIZE_LIMIT" + gradle :test:partiql-tests-runner:run --args="$ARGS" # Find comment w/ conformance comparison if previous comment published - name: Find Comment uses: peter-evans/find-comment@v2 @@ -120,5 +133,5 @@ jobs: with: comment-id: ${{ steps.fc.outputs.comment-id }} issue-number: ${{ github.event.pull_request.number }} - body-file: ${{ env.PATH_TO_TEST_RUNNER }}/${{ env.COMPARISON_REPORT_NAME }} + body-file: ${{ env.PATH_TO_TEST_RUNNER }}/${{ env.COMPARISON_REPORT_NAME_WITH_LIMIT }} edit-mode: replace diff --git a/test/partiql-tests-runner/build.gradle.kts b/test/partiql-tests-runner/build.gradle.kts index 8246f8f30f..9fa06449da 100644 --- a/test/partiql-tests-runner/build.gradle.kts +++ b/test/partiql-tests-runner/build.gradle.kts @@ -31,6 +31,7 @@ dependencies { } val tests = System.getenv()["PARTIQL_TESTS_DATA"] ?: "../partiql-tests/partiql-tests-data" +val reportDir = file("$buildDir/conformance-test-report").absolutePath object Env { const val PARTIQL_EVAL = "PARTIQL_EVAL_TESTS_DATA" @@ -49,16 +50,19 @@ tasks.test { exclude("org/partiql/runner/ConformanceTest.class") } -tasks.register("ConformanceTestReport") { - val reportDir = file("$buildDir/conformance-test-report").absolutePath +val createReportDir by tasks.registering { if (File(reportDir).exists()) { delete(File(reportDir)) } + mkdir(reportDir) +} + +val generateTestReport by tasks.registering(Test::class) { + dependsOn(createReportDir) useJUnitPlatform() environment(Env.PARTIQL_EVAL, file("$tests/eval/").absolutePath) environment(Env.PARTIQL_EQUIV, file("$tests/eval-equiv/").absolutePath) environment("conformanceReportDir", reportDir) - mkdir(reportDir) include("org/partiql/runner/ConformanceTestEval.class", "org/partiql/runner/ConformanceTestLegacy.class") if (project.hasProperty("Engine")) { val engine = property("Engine")!! as String diff --git a/test/partiql-tests-runner/src/main/kotlin/org/partiql/runner/ConformanceComparison.kt b/test/partiql-tests-runner/src/main/kotlin/org/partiql/runner/ConformanceComparison.kt index 23a554fd70..522c44848f 100644 --- a/test/partiql-tests-runner/src/main/kotlin/org/partiql/runner/ConformanceComparison.kt +++ b/test/partiql-tests-runner/src/main/kotlin/org/partiql/runner/ConformanceComparison.kt @@ -4,9 +4,9 @@ import com.amazon.ionelement.api.loadSingleElement import java.io.File fun main(args: Array) { - if (args.size != 5) { + if (args.size < 5) { error( - "Expected 5 args: pathToFirstConformanceTestResults, pathToSecondConformanceTestResults" + + "Expected at least 5 args: pathToFirstConformanceTestResults, pathToSecondConformanceTestResults" + "firstCommitId, secondCommitId, pathToComparisonReport" ) } @@ -20,18 +20,20 @@ fun main(args: Array) { val oldReports = loadReport(old, oldCommitId) val newReports = loadReport(new, newCommitId) val comparisonReportFile = File(args[4]) + val limit = if (args.size == 6) args[5].toInt() else Int.MAX_VALUE comparisonReportFile.createNewFile() - analyze(comparisonReportFile, newReports) + // cross engine + analyze(comparisonReportFile, newReports, limit) val all = oldReports + newReports - // cross commit comparsion + // cross commit comparison all .groupBy { it.engine } .forEach { (_, reports) -> - analyze(comparisonReportFile, reports) + analyze(comparisonReportFile, reports, limit) } } data class Report( @@ -42,11 +44,11 @@ data class Report( val ignoredSet: Set ) -fun analyze(file: File, reports: List) { +fun analyze(file: File, reports: List, limit: Int) { var first = 0 var second = first + 1 while (first < second && second < reports.size) { - val report = ReportAnalyzer.build(reports[first], reports[second]).generateComparisonReport() + val report = ReportAnalyzer.build(reports[first], reports[second]).generateComparisonReport(limit) file.appendText(report) file.appendText("\n") if (second < reports.size - 1) { @@ -59,12 +61,14 @@ fun analyze(file: File, reports: List) { } fun loadReport(dir: File, commitId: String) = - dir.listFiles()?.map { sub -> - val engine = sub.name - val report = - (sub.listFiles() ?: throw IllegalArgumentException("sub-dir ${sub.absolutePath} not exist")).first().readText() - loadReport(report, engine, commitId) - } ?: throw IllegalArgumentException("dir ${dir.absolutePath} not exist") + dir.listFiles() + ?.filter { it.isDirectory } + ?.map { sub -> + val engine = sub.name + val report = + (sub.listFiles() ?: throw IllegalArgumentException("sub-dir ${sub.absolutePath} not exist")).first().readText() + loadReport(report, engine, commitId) + } ?: throw IllegalArgumentException("dir ${dir.absolutePath} not exist") fun loadReport(report: String, engine: String, commitId: String): Report { val inputStruct = loadSingleElement(report).asStruct() @@ -101,13 +105,13 @@ abstract class ReportAnalyzer(first: Report, second: Report) { val secondPassingPercent = secondPassingSize.toDouble() / secondTotalSize * 100 abstract val reportTitle: String - abstract fun generateComparisonReport(): String + abstract fun generateComparisonReport(limit: Int): String } class CrossCommitReportAnalyzer(private val first: Report, private val second: Report) : ReportAnalyzer(first, second) { override val reportTitle: String = "Conformance comparison report-Cross Commit-${first.engine.uppercase()}" - override fun generateComparisonReport() = + override fun generateComparisonReport(limit: Int) = buildString { this.appendLine( """### $reportTitle @@ -134,7 +138,7 @@ Number failing in Base (${first.commitId}) but now pass: ${failureFirstPassingSe if (passingFirstFailingSecond.isNotEmpty()) { // character count limitation with comments in GitHub // also, not ideal to list out hundreds of test names - if (passingFirstFailingSecond.size < 10) { + if (passingFirstFailingSecond.size < limit) { this.appendLine(":interrobang: CONFORMANCE REPORT REGRESSION DETECTED :interrobang:. The following test(s) were previously passing but now fail:\n
Click here to see\n\n") passingFirstFailingSecond.forEach { testName -> @@ -143,11 +147,12 @@ Number failing in Base (${first.commitId}) but now pass: ${failureFirstPassingSe this.appendLine("
") } else { this.appendLine(":interrobang: CONFORMANCE REPORT REGRESSION DETECTED :interrobang:") + this.appendLine("The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.") } } if (failureFirstPassingSecond.isNotEmpty()) { - if (failureFirstPassingSecond.size < 10) { + if (failureFirstPassingSecond.size < limit) { this.appendLine( "The following test(s) were previously failing but now pass. Before merging, confirm they are intended to pass: \n
Click here to see\n\n" ) @@ -157,6 +162,7 @@ Number failing in Base (${first.commitId}) but now pass: ${failureFirstPassingSe this.appendLine("
") } else { this.appendLine("${failureFirstPassingSecond.size} test(s) were previously failing but now pass. Before merging, confirm they are intended to pass") + this.appendLine("The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.") } } } @@ -164,7 +170,7 @@ Number failing in Base (${first.commitId}) but now pass: ${failureFirstPassingSe class CrossEngineReportAnalyzer(private val first: Report, private val second: Report) : ReportAnalyzer(first, second) { override val reportTitle: String = "Conformance comparison report-Cross Engine" - override fun generateComparisonReport() = + override fun generateComparisonReport(limit: Int) = buildString { this.appendLine( """### $reportTitle @@ -189,7 +195,7 @@ Number failing in ${first.engine} engine but pass in ${second.engine} engine: ${ """.trimIndent() ) if (passingFirstFailingSecond.isNotEmpty()) { - if (passingFirstFailingSecond.size < 10) { + if (passingFirstFailingSecond.size < limit) { this.appendLine(":interrobang: CONFORMANCE REPORT REGRESSION DETECTED :interrobang:. The following test(s) are passing in ${first.engine} but fail in ${second.engine}:\n
Click here to see\n\n") passingFirstFailingSecond.forEach { testName -> @@ -198,11 +204,12 @@ Number failing in ${first.engine} engine but pass in ${second.engine} engine: ${ this.appendLine("
") } else { this.appendLine(":interrobang: CONFORMANCE REPORT REGRESSION DETECTED :interrobang:") + this.appendLine("The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.") } } if (failureFirstPassingSecond.isNotEmpty()) { - if (failureFirstPassingSecond.size < 10) { + if (failureFirstPassingSecond.size < limit) { this.appendLine( "The following test(s) are failing in ${first.engine} but pass in ${second.engine}. Before merging, confirm they are intended to pass: \n
Click here to see\n\n" ) @@ -212,6 +219,7 @@ Number failing in ${first.engine} engine but pass in ${second.engine} engine: ${ this.appendLine("
") } else { this.appendLine("${failureFirstPassingSecond.size} test(s) were failing in ${first.engine} but now pass in ${second.engine}. Before merging, confirm they are intended to pass.") + this.appendLine("The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.") } } }