Skip to content

improve conformance comparison report for new tests #1833

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Oct 2, 2025
Merged

improve conformance comparison report for new tests #1833

merged 9 commits into from
Oct 2, 2025

Conversation

xd1313113
Copy link
Contributor

@xd1313113 xd1313113 commented Sep 26, 2025 •
edited
Loading

Relevant Issues

Description

  • < Explain >

Other Information

  • Updated Unreleased Section in CHANGELOG: NO: Nothing to release
  • Any backward-incompatible changes? No
  • Any new external dependencies? NO
  • Do your changes comply with the contributing and code style guidelines? YES

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 26, 2025 •
edited
Loading

CROSS-COMMIT-REPORT-PARTIQLCORE ✅

BASE (8D2859F) TARGET (77FC78B) +/-
% Passing 94.65% 0.00% -94.65% ⭕
Passing 6440 0 -6440 ⭕
Failing 176 0 -176 ✅
Ignored 188 0 -188 ✅
Total Tests 6804 0 -6804 ⭕

Testing Details

Result Details

  • New passing tests: 0
  • New failing tests: 0
  • New ignored tests: 0
  • Passing in both: 0
  • Failing in both: 0
  • Ignored in both: 0
  • PASSING in BASE but now FAILING in TARGET: 0
  • PASSING in BASE but now IGNORED in TARGET: 0
  • FAILING in BASE but now PASSING in TARGET: 0
  • IGNORED in BASE but now PASSING in TARGET: 0

CROSS-COMMIT-REPORT-PARTIQLEXTENDED ✅

BASE (8D2859F) TARGET (77FC78B) +/-
% Passing 34.25% 0.00% -34.25% ⭕
Passing 670 0 -670 ⭕
Failing 1286 0 -1286 ✅
Ignored 0 0 0 ✅
Total Tests 1956 0 -1956 ⭕

Testing Details

Result Details

  • New passing tests: 0
  • New failing tests: 0
  • New ignored tests: 0
  • Passing in both: 0
  • Failing in both: 0
  • Ignored in both: 0
  • PASSING in BASE but now FAILING in TARGET: 0
  • PASSING in BASE but now IGNORED in TARGET: 0
  • FAILING in BASE but now PASSING in TARGET: 0
  • IGNORED in BASE but now PASSING in TARGET: 0

@xd1313113 xd1313113 mentioned this pull request Sep 29, 2025
Merged
@xd1313113
Copy link
Contributor Author

xd1313113 commented Oct 1, 2025 •
edited
Loading

A quick link to the sample full conformance report
https://github.com/partiql/partiql-lang-kotlin/actions/runs/18176496528/artifacts/4159488336

@xd1313113 xd1313113 marked this pull request as ready for review October 1, 2025 22:27
alancai98
alancai98 reviewed Oct 1, 2025
View reviewed changes
Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

Please also update the PR title and description.

test/partiql-tests-runner/src/main/kotlin/org/partiql/runner/ReportAnalyzer.kt Outdated
out.appendLine("- **Base Commit**: ${first.commitId}")
out.appendLine("- **Target Commit**: ${second.commitId}")
out.appendLine("- **Java Version**: ${VersionProvider.getJavaVersion()}")
out.appendLine("- **PartiQL Version**: ${VersionProvider.getPartiQLVersion()}")
Copy link
Member

Choose a reason for hiding this comment

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

I can see value in printing the Java version but the PartiQL version is somewhat redundant since we already print out the base and target commits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is listed in Dod of #1756. It should be fine as the version string is more straightforward.

Copy link
Member

@alancai98 alancai98 Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There's no DoD for #1756. The "Additional Context" section that includes the java and partiql versions comes from the bug-report template -- https://github.com/partiql/partiql-lang-kotlin/blob/main/.github/ISSUE_TEMPLATE/bug-report.md#additional-context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I misunderstand the info. Just added java version as it might may helpful

test/partiql-tests-runner/src/test/kotlin/org/partiql/runner/ConformanceTestBase.kt
Comment on lines +3 to +6
import org.junit.jupiter.api.TestInstance
import org.partiql.runner.test.TestRunner

@TestInstance(TestInstance.Lifecycle.PER_CLASS) // Annotation is need to avoid skipList being recreated for each test.
Copy link
Member

Choose a reason for hiding this comment

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

Was this an issue before?

Copy link
Contributor Author

@xd1313113 xd1313113 Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

No real issue for now. Can save a bit of time. Do you want to leave it as it was or is It Ok

XuechunHHH
XuechunHHH reviewed Oct 2, 2025
View reviewed changes
test/partiql-tests-runner/src/main/kotlin/org/partiql/runner/ReportAnalyzer.kt Outdated
private val passingFirstIgnoredSecond = first.passingSet.intersect(second.ignoredSet)
private val failureFirstPassingSecond = first.failingSet.intersect(second.passingSet)
private val ignoredFirstPassingSecond = first.ignoredSet.intersect(second.passingSet)
private val newPassing = second.passingSet - first.passingSet - first.failingSet - first.ignoredSet
Copy link
Contributor

@XuechunHHH XuechunHHH Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am wondering why it shouldn't be newPassing = second.passingSet - first.passingSet instead. e.g., test a failed in the source (in first.failingSet) but pass in target (in second.passingSet). Should a be a newPassing test in this case? Same for newFailing and newIgnored ones.

But the sample report result seems work well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are collecting new tests added only. Anything in old test should not be included

@xd1313113 xd1313113 changed the title Add duplicate detection and improve report for new tests improve conformance comparison report for new tests Oct 2, 2025
@xd1313113 xd1313113 merged commit 6792e1d into main Oct 2, 2025
17 checks passed
@alancai98 alancai98 mentioned this pull request Oct 3, 2025
Closed
@alancai98 alancai98 linked an issue Oct 3, 2025 that may be closed by this pull request
Conformance report issues #1756
Closed
@alancai98 alancai98 deleted the dixiao/reportfix branch October 3, 2025 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alancai98 alancai98 alancai98 approved these changes

@XuechunHHH XuechunHHH Awaiting requested review from XuechunHHH

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Conformance report issues
3 participants
@xd1313113 @alancai98 @XuechunHHH