Skip to content

Revised a few existing tests that are flawed #1103

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

Merged
merged 3 commits into from
Mar 2, 2020
Merged

Revised a few existing tests that are flawed #1103

merged 3 commits into from
Mar 2, 2020

Conversation

yitam
Copy link
Contributor

@yitam yitam commented Feb 25, 2020

While converting tests from PR #720 discovered that the designs were flawed. These tests are rewritten and expanded to include non-AE testing as well.

@coveralls
Copy link

coveralls commented Feb 25, 2020

Coverage Status

Coverage increased (+0.009%) to 74.445% when pulling 6827c6c on yitam:outputTests into cd64173 on microsoft:dev.

return $success;
}

function isCompatible($dataType, $sqlType)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same function as in the previous test? Can we put this in a common include file, maybe without $compatList as an argument?

// return SQLSTATE '42000' or '22018', the former indicating
// an error when converting from one type to another and the
// latter indicating an incompatible conversion error
// Either one is acceptable for the purpose of 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.

Wait, why would we expect errors with compatible types?

createProc($conn, $spname, "@c_det $dataType OUTPUT, @c_rand $dataType OUTPUT", "SELECT @c_det = c_det, @c_rand = c_rand FROM $tbname");

// insert a row
$inputValues = array_slice(${explode("(", $dataType)[0] . "_params"}, 1, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have an example here of what the input looks like? Expressions like this are tricky to parse by looking.

$r;
$stmt = AE\insertRow($conn, $tbname, array( $colMetaArr[0]->colName => $inputValues[0], $colMetaArr[1]->colName => $inputValues[1] ), $r);
if ($r === false) {
is_incompatible_types_error($dataType, "default type");
Copy link
Contributor

Choose a reason for hiding this comment

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

is_incompatible_types_error has the same issue you fixed elsewhere - it compares SQLSTATE to a non-string 22018.

// clash but only happens with some certain types)
// E.g. when converting a bigint to int or an int to numeric,
// SQLSTATE '42000' is returned, indicating an error when
// converting from one type to another.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not clear on why we would get errors if the types are supposed to be compatible. Do the errors occur only in AE?

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 only happens with non-AE, and there are many possible combinations -- will investigate further in another task

@yitam yitam merged commit 8bb6cef into microsoft:dev Mar 2, 2020
@yitam yitam deleted the outputTests branch March 2, 2020 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants