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.

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

Add conformance tests for checking expression #1069

Closed
wants to merge 10 commits into from
Closed

Add conformance tests for checking expression #1069

wants to merge 10 commits into from

Conversation

SandaruJayawardana
Copy link
Contributor

Purpose

$title.

Related lang issue #34999

gimantha
gimantha reviewed Mar 23, 2022
View reviewed changes
conformance/lang/expressions/checking-expression/checkpanic_expr.balt
}

function fn1() returns error? {
string a1 = checkpanic (string`${12 + 212 - 0}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we also write some tests using checking expressions inside the string template?

gimantha
gimantha reviewed Mar 23, 2022
View reviewed changes
conformance/lang/expressions/checking-expression/checkpanic_expr.balt
}

function listConstructor() returns error? {
int[] a1 = checkpanic [1, 2, 3, -0x12, int:MIN_VALUE];
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we add test cases like,

int[] a1 = checkpanic [1, 2, 0/0, -0x12, int:MIN_VALUE];

int[] a1 = [1, 2, checkpanic 0/0, -0x12, int:MIN_VALUE];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, checkpanic is not necessary because 0/0 causes to panic the code. will add this with trap.

gimantha
gimantha reviewed Mar 23, 2022
View reviewed changes
conformance/lang/expressions/checking-expression/checkpanic_expr.balt

function fn() returns error {
io:println("function");
return error("error1!");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return error("error1!");
return error("error!");

@gimantha
Copy link
Contributor

gimantha commented Mar 23, 2022 •
edited
Loading

I think we can add few more cases

  • checking expressions inside other control structures like foreach, while statements..
  • checking expressions inside different clauses of query expressions and query actions
  • checking expressions guarded with fail block
  • checking expressions for custom error types
  • Also to verify the behavior checking expression with ensureType in lax case

@SandaruJayawardana SandaruJayawardana changed the base branch from conformance-tests to master April 19, 2022 02:33
@KavinduZoysa KavinduZoysa requested review from rdulmina and gimantha May 26, 2022 04:35
ushirask
ushirask reviewed Jun 2, 2022
View reviewed changes
conformance/lang/expressions/checking-expression/check_expr.balt Outdated
mapping-constructor-expr, nil-type, string, function-defn, function-call-expr, union-type, optional-type

function errorFunction() {
error? x1 = listConstructor(); // @error expected '(int|error)' but found '[int,int,int,int,int]'
Copy link

Choose a reason for hiding this comment

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

Similar to above.

rdulmina
rdulmina reviewed Jun 3, 2022
View reviewed changes
conformance/lang/expressions/checking-expression/check_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/check_expr.balt
Comment on lines +62 to +66
error? x1 = fn1(); // @output 123
// @output 9223372036854775807
// @output -9223372036854775808
// @output 1223617
// @output -11256097
Copy link
Contributor

Choose a reason for hiding this comment

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

Variables are not used. Can we inline these outputs with io:println lines. One solution is to introduce a printResult() function at the bottom and pass the result x1 to it and print it there @SandaruJayawardana @MaryamZi WDYT?

conformance/lang/expressions/checking-expression/check_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/check_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/check_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/check_expr.balt Outdated
Comment on lines 551 to 569
class C1 {
C2 c2 = new();

public function method1(int i) returns error? {
check self.c2.method2(i);
io:println("No errors");
}
}

class C2 {
public int x = 1;

public function method2(int i) returns error? {
if i == 0 {
return;
}
return error("mothod2");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simplify this test using one object constructor and removing class defn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will check

conformance/lang/expressions/checking-expression/check_expr.balt
Comment on lines +620 to +622
if i >= 0 {
return i;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the tests as minimalized as possible and remove these logics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1. But I think we need a simple logic to verify the functionality of inner and outer check-expr in this test.

Copy link
Contributor

Choose a reason for hiding this comment

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

just returning the error in fn1 and fn2 is enough right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case we cannot test whether the check expression properly work for non-error scenarios.

conformance/lang/expressions/checking-expression/check_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/check_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/check_expr.balt Outdated Show resolved Hide resolved
ushirask
ushirask reviewed Jun 3, 2022
View reviewed changes
conformance/lang/expressions/checking-expression/checkpanic_expr.balt Show resolved Hide resolved
conformance/lang/expressions/checking-expression/checkpanic_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/checkpanic_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/checkpanic_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/checkpanic_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/checkpanic_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/checkpanic_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/checkpanic_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/checkpanic_expr.balt Outdated Show resolved Hide resolved
conformance/lang/expressions/checking-expression/checkpanic_expr.balt Outdated Show resolved Hide resolved
@hasithaa hasithaa closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sameerajayasoma sameerajayasoma Awaiting requested review from sameerajayasoma sameerajayasoma is a code owner

@hasithaa hasithaa Awaiting requested review from hasithaa hasithaa is a code owner

@rdhananjaya rdhananjaya Awaiting requested review from rdhananjaya

@MaryamZi MaryamZi Awaiting requested review from MaryamZi MaryamZi is a code owner

@gimantha gimantha Awaiting requested review from gimantha gimantha is a code owner

@ushirask ushirask Awaiting requested review from ushirask

@rdulmina rdulmina Awaiting requested review from rdulmina

Assignees

@gimantha gimantha

Labels
None yet
Projects
No open projects
Compiler Core Pull Requests
Status: Waiting on Reviewers
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SandaruJayawardana @gimantha @rdulmina @ushirask @hasithaa