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
Add SQLTest #52293
Add SQLTest #52293
Conversation
This is an automated comment for commit 561e043 with description of existing statuses. It's updated for the latest CI running
|
set -u | ||
set -o pipefail | ||
|
||
BINARY_TO_DOWNLOAD=${BINARY_TO_DOWNLOAD:="clang-16_debug_none_unsplitted_disable_False_binary"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, don't use the worst practices from performance tests. Binaries should be downloaded in advance and mounted into the container by a script from tests/ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I use copy-paste and ChatGPT to write Python. And when I'm using copy-paste, I'm trying to reuse the shortest code, so I have less amount to read.
|
||
run_log_path = os.path.join(temp_path, "run.log") | ||
with open(run_log_path, "w", encoding="utf-8") as log: | ||
with subprocess.Popen( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TeePopen
looks like a better class here
tests/ci/sqltest.py
Outdated
s3_prefix = f"{pr_info.number}/{pr_info.sha}/sqltest_{check_name_lower}/" | ||
paths = { | ||
"run.log": run_log_path, | ||
"main.log": os.path.join(workspace_path, "main.log"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a side note, py3 has pathlib.Path
class. It's much nicer in usage than the old os.path
things
s3_helper = S3Helper() | ||
for f in paths: | ||
try: | ||
paths[f] = s3_helper.upload_test_report_to_s3(paths[f], s3_prefix + f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by adding logs to TestResult
you can skip this part
|
||
status = "success" | ||
description = "See the report" | ||
test_result = TestResult(description, "OK") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add logs and report files here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I want to make a small working prototype first.
This check has limited use. It should exist; everything else we will address later (or never).
@Felixoid currently it does not work due to
|
What is the difference between |
I found something strange, but didn't understand it: #33751 |
@Felixoid Now it is complete ..F.........
|
You shouldn't have used |
I thought Azure test was fixed... |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add https://github.com/elliotchance/sqltest to CI to report the SQL 2016 conformance.