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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import static org.ballerinalang.test.TestRunnerUtils.handleTestSkip;
import static org.ballerinalang.test.TestRunnerUtils.setDetails;
import static org.ballerinalang.test.TestRunnerUtils.setDetailsOfTest;
import static org.ballerinalang.test.TestRunnerUtils.validateLabels;
import static org.ballerinalang.test.TestRunnerUtils.validateTestFormat;
import static org.ballerinalang.test.TestRunnerUtils.validateTestOutput;

Expand All @@ -53,14 +54,16 @@ public class TestRunner {

private final Path path = TEST_DIR.resolve("src").resolve("test").resolve("resources")
.resolve("ballerina-spec-tests").resolve("conformance");
private static final Set<String> predefinedLabels = TestRunnerUtils.readLabels(TEST_DIR.toString()).keySet();

@Test(dataProvider = "spec-conformance-tests-file-provider")
public void test(String kind, String path, List<String> outputValues, List<Integer> lineNumbers, String fileName,
int absLineNum, boolean isSkippedTest, String diagnostics, ITestContext context,
boolean isKnownIssue) {
boolean isKnownIssue, String labels) {
setDetailsOfTest(context, kind, fileName, absLineNum, diagnostics);
handleTestSkip(isSkippedTest);
validateTestFormat(diagnostics);
validateLabels(labels, predefinedLabels, absLineNum);
validateTestOutput(path, kind, outputValues, isKnownIssue, lineNumbers, fileName, absLineNum, context);
}

Expand Down Expand Up @@ -90,8 +93,6 @@ private HashSet<String> runSelectedTests(HashMap<String, HashSet<String>> define

@DataProvider(name = "spec-conformance-tests-file-provider")
public Iterator<Object[]> dataProvider() {
HashMap<String, HashSet<String>> definedLabels = TestRunnerUtils.readLabels(TEST_DIR.toString());
Set<String> labels = runSelectedTests(definedLabels);
List<Object[]> testCases = new ArrayList<>();
try {
Files.walk(path).filter(path -> {
Expand All @@ -101,7 +102,8 @@ public Iterator<Object[]> dataProvider() {
.map(path -> new Object[]{path.toFile().getName(), path.toString()})
.forEach(object -> {
try {
TestRunnerUtils.readTestFile((String) object[0], (String) object[1], testCases, labels);
TestRunnerUtils.readTestFile((String) object[0], (String) object[1], testCases,
predefinedLabels);
} catch (IOException e) {
Assert.fail("failed to read spec conformance test: \"" + object[0] + "\"", e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.StringJoiner;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -110,16 +111,18 @@ private static void readTestFile(File testFile, String tempDir, String fileName,

Map<String, String> headersOfTestCase = readHeaders(line, buffReader);
String kindOfTestCase = validateKindOfTest(headersOfTestCase.get(TEST_CASE));

//TODO: if kind of testcase is other, then need to skip creating the bal file
boolean isSkippedTestCase = isSkippedTestCase(selectedLabels, headersOfTestCase.get(LABELS));

Object[] testCase = new Object[9];
Object[] testCase = new Object[10];
testCase[0] = kindOfTestCase;
testCase[1] = tempDir + tempFileName + BAL_EXTENSION;
testCase[4] = fileName;
testCase[5] = absLineNum;
testCase[6] = isSkippedTestCase;
testCase[8] = headersOfTestCase.containsKey(FAIL_ISSUE);
testCase[9] = headersOfTestCase.get(LABELS);

line = writeToBalFile(testCases, testCase, kindOfTestCase, tempDir, tempFileName, buffReader);
}
Expand Down Expand Up @@ -196,6 +199,36 @@ private static String validateKindOfTest(String kind) {
}
}

public static void validateLabels(String labels, Set<String> predefinedLabels, int absLineNum) {
HashSet<String> labelsList = new HashSet<>();
StringJoiner duplicateLabels = new StringJoiner(", ");
StringJoiner unknownLabels = new StringJoiner(", ");

for (String label : labels.split(",")) {
String trimmedLabel = label.trim();
if (labelsList.contains(trimmedLabel)) {
duplicateLabels.add(trimmedLabel);
continue;
}
if (!predefinedLabels.contains(trimmedLabel)) {
unknownLabels.add(trimmedLabel);
}
labelsList.add(trimmedLabel);
}

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

"Unknown labels: " + unknownLabels + "\n Duplicate labels: " + duplicateLabels :
"Unknown labels: " + unknownLabels) :
(hasDuplicateLabels ? "Duplicate labels: " + duplicateLabels : "");

if (!diagnosticMsg.isEmpty()) {
Assert.fail("Errors in labels at line number: " + (absLineNum - 1) + "\n" + diagnosticMsg);
}
}

private static Map<String, String> readHeaders(String line, BufferedReader buffReader) throws IOException {
ArrayList<String> requiredHeaders = new ArrayList<>(Arrays.asList(TEST_CASE, DESCRIPTION, LABELS));
Map<String, String> headers = new HashMap<>();
Expand Down Expand Up @@ -496,7 +529,7 @@ public static Map<String, String> getDetailsOfTest(ITestContext context, ITestRe
public static void setDetailsOfErrorKindTests(ITestContext context, Map<String, String> detailsOfTest) {
Map<String, Object> detailsOfErrorKindTests = getDetailsOfErrorKindTests(context);

boolean haveOnlyNulls = detailsOfErrorKindTests.values().stream().allMatch(Objects::isNull);;
boolean haveOnlyNulls = detailsOfErrorKindTests.values().stream().allMatch(Objects::isNull);

if (!haveOnlyNulls) {
int absLineNo = Integer.parseInt(detailsOfTest.get(ABS_LINE_NUM));
Expand Down
216 changes: 162 additions & 54 deletions tests/ballerina-spec-conformance-tests/src/test/resources/labels.csv
Original file line number Diff line number Diff line change
@@ -1,72 +1,180 @@
Label,Implies
any
boolean
byte
decimal
float
int
null
panic
string
var
any,
boolean,
byte,
decimal,
float,
int,
null,
panic,
string,
var,
int:Signed32,int
int:Unsigned32,int
int:abs,int
int:max,int
int:MAX_VALUE,int
int:MIN_VALUE,int
value:toBalString
value:toBalString,
float:PI,float
equality
exact-equality
equality,
exact-equality,
infinity,float
nan,float
numeric-nil-lifting
numeric-nil-lifting,
panic-arithmetic-overflow,panic
signed-zero
signed-zero,
StringNumericEscape,StringEscape
unary-complement
unary-minus
unary-not
unary-plus
additive-expr
binary-bitwise-expr
unary-complement,
unary-minus,
unary-not,
unary-plus,
additive-expr,
binary-bitwise-expr,
byte-array-literal,"byte, array-type"
DecimalTypeSuffix,decimal
FloatTypeSuffix,float
HexFloatingPointLiteral,float
HexIntLiteral,int
is-expr
logical-expr
module-type-defn
multiplicative-expr
nil-literal
shift-expr
is-expr,
logical-expr,
module-type-defn,
multiplicative-expr,
nil-literal,
shift-expr,
StringEscape,string
type-cast-expr
type-cast-expr,
raw-template-expr,BacktickString
while-stmt,boolean
BacktickString
array-type
map-type
optional-type
record-type
singleton-type
tuple-type
union-type
nil-type
mapping-constructor-expr
list-constructor-expr
table-constructor-expr
intersection-type
readonly-type
member-access-expr
foreach-stmt
method-call-expr
match-stmt
iterable-mapping
iterable-list
iterable-exclusive
iterable-inclusive
iterable-string
iterable-object
BacktickString,
array-type,
map-type,
optional-type,
record-type,
singleton-type,
tuple-type,
union-type,
nil-type,
mapping-constructor-expr,
list-constructor-expr,
table-constructor-expr,
intersection-type,
readonly-type,
member-access-expr,
foreach-stmt,
iterable-mapping,
iterable-list,
iterable-exclusive,
iterable-inclusive,
iterable-string,
iterable-object,
method-call-expr,
match-stmt,
int:SIGNED32_MAX_VALUE,int:Signed32
int:SIGNED32_MIN_VALUE,int:Signed32
int:SIGNED16_MAX_VALUE,int:Signed16
int:SIGNED16_MIN_VALUE,int:Signed16
int:SIGNED8_MAX_VALUE,int:Signed8
int:SIGNED8_MIN_VALUE,int:Signed8
int:UNSIGNED32_MAX_VALUE,int:Unsigned32
int:UNSIGNED16_MAX_VALUE,int:Unsigned16
int:UNSIGNED8_MAX_VALUE,int:Unsigned8
int:sum,int
int:min,int
int:fromString,int
int:toHexString,int
int:fromHexString,int
int:Signed8,int
int:Unsigned8,int
int:Signed16,int
int:Unsigned16,int
function-defn,
function-call-expr,
local-init-var-decl-stmt,
variable-reference-expr,
if-else-stmt,
boolean-literal,
DecimalNumber,
return-stmt,
StringSingleEscape,StringEscape
defaultable-param,function-signature
field-access-expr,
optional-field-access-expr,
module-class-defn,
implicit-new-expr,new-expr
explicit-new-expr,new-expr
check,checking-expr
checkpanic,checking-expr
json,
xml,
xml:Element,xml
xml:ProcessingInstruction,xml
xml:Comment,xml
xml:Text,xml
xml-type-parameter,xml
module-const-decl,
lax-typing,
computed-name-field,mapping-constructor-expr
spread-field,mapping-constructor-expr
DecimalFloatingPointNumber,
ternary-conditional-expr,conditional-expr
binary-conditional-expr,conditional-expr
let-expr,
relational-expr,
anydata,
FloatingPointTypeSuffix,
string:Char,string
error-constructor-expr,
error-type,
error-type-parameter,error
never,
record-type-inclusion,
readonly-field,record-type
module-init-var-decl,
module-no-init-var-decl,
record-rest-descriptor,record-type
string-template-expr,"BacktickString, string"
xml-template-expr,"BacktickString, xml"
object-type,
object-constructor-expr,object-type
xml-required-attribute-access-expr,
xml-optional-attribute-access-expr,
annot-access-expr,
annotation-decl,
annotation,
explicit-anonymous-function-expr,
infer-anonymous-function-expr,
function-type,
typeof-expr,
trap-expr,
query-expr,
transactional-expr,
xml-filter-expr,
xml-step-expr,
table-type,
key-specifier,
xmlns-decl,
typedesc-type,
range-expr,
transaction-stmt,
retry-transaction-stmt,
commit-action,
future,
future-type-parameter,
distinct,
handle,
stream,
stream-type-parameters,
transactional-function,
transactional-method,
transactional-worker,
start-action,
const-reference-expr,
type-reference,
string:length,
xml:length,
rollback-stmt,
transaction:onRollback,
isolated-function,
transaction:Info,
error:Retriable,
compound-assignment-stmt,
transaction:onCommit