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

Support validating test labels using the test runner #35308

Conversation

lasinicl
Copy link
Contributor

@lasinicl lasinicl commented Mar 3, 2022

Purpose

$title

Fixes #33691

Approach

Validates whether the labels defined under the Labels header of the test case are among the predefined set of labels or else such labels are identified as "unknown labels".
Also, check whether there are duplicate labels.
We can't distinguish between the typos and the unknown labels.

Samples

Provide high-level details about the samples related to this feature.

Remarks

List any other known issues, related PRs, TODO items, or any other notes related to the PR.

Check List

  • Read the Contributing Guide
  • Updated Change Log
  • Checked Tooling Support (#)
  • Added necessary tests
    • Unit Tests
    • Spec Conformance Tests
    • Integration Tests
    • Ballerina By Example Tests
  • Increased Test Coverage
  • Added necessary documentation
    • API documentation
    • Module documentation in Module.md files
    • Ballerina By Examples

@lasinicl lasinicl added the Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. label Mar 3, 2022
@pcnfernando pcnfernando self-assigned this Mar 4, 2022
Copy link
Member

@lochana-chathura lochana-chathura left a comment

Choose a reason for hiding this comment

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

With this, there seem to be failing tests due to the unknown, duplicate labels. Maybe we can fix them separately. Hence approving.

boolean hasUnknownLabels = unknownLabels.length() > 0;
boolean hasDuplicateLabels = duplicateLabels.length() > 0;

String diagnosticMsg = hasUnknownLabels ? (hasDuplicateLabels ?
Copy link
Member

Choose a reason for hiding this comment

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

Shall we propagate the line number here and log it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the line number of the test case or the line number of the label? We don't seem to keep information on the line numbers of the labels ATM though. Will need to add support for this if that's the requirement here.

Copy link
Member

Choose a reason for hiding this comment

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

Line number in the test case. Specifically .balt file.
Isn't absLineNum argument TestRunner.test() represent that?

Copy link
Member

@lochana-chathura lochana-chathura Mar 9, 2022

Choose a reason for hiding this comment

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

Data provider provides the absLineNum. The line number of the last row of the labels will be absLineNum - 1 right?
If there's more than one row of labels, right now we can't pinpoint the row.
But for now, we can log the error to the last line in case of the label list contains duplicates/unknowns.

Copy link
Contributor Author

@lasinicl lasinicl Mar 9, 2022

Choose a reason for hiding this comment

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

Yeah. But anyway we can see which test has failed right?. Then, we can just check and correct the label there. If we can't pinpoint the row I don't think it makes any difference, even if log the error to the last line since we'll anyway have to find where the label exactly is.

Copy link
Member

Choose a reason for hiding this comment

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

Pin-pointing the raw is not needed. Just the line number will be useful as I see it. It's matter of checking the label string in the test.
Does the failed test pin-point to the specific line on the original balt. No right?
I think logging the absLineNum would be enough for the moment and it will be really useful. Theat's what I felt trying out your changes against .balt files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, added the line number in 7f9e8ab

Copy link
Member

@pcnfernando pcnfernando left a comment

Choose a reason for hiding this comment

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

LGTM

@pcnfernando pcnfernando merged commit f1dbac1 into ballerina-platform:spec-conformance-test-runner Mar 9, 2022
@lasinicl lasinicl deleted the spec-conformance-test-runner branch March 9, 2022 08:29
@MaryamZi
Copy link
Member

@lasinicl, is there a way to figure out test failures due to invalid labels from any of the reports? Without going through the logs? If not, IMO, we need to introduce/improve a report to include this.

Right now, the failed test summary is also misleading when tests fail due to unknown labels.

Let's create an issue for this too.

@lasinicl
Copy link
Contributor Author

lasinicl commented Mar 14, 2022

@lasinicl, is there a way to figure out test failures due to invalid labels from any of the reports? Without going through the logs? If not, IMO, we need to introduce/improve a report to include this.

Right now, the failed test summary is also misleading when tests fail due to unknown labels.

Let's create an issue for this too.

Created #35454

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times.
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants