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

Connector Test Failure reported as success on failure #27425

Closed
bnchrch opened this issue Jun 15, 2023 · 5 comments · Fixed by #28789
Closed

Connector Test Failure reported as success on failure #27425

bnchrch opened this issue Jun 15, 2023 · 5 comments · Fixed by #28789
Assignees
Labels

Comments

@bnchrch
Copy link
Contributor

bnchrch commented Jun 15, 2023

Problem

source-firebase-realtime-database was reported as passing in the github comment when it had failed
#27420
image.png
image.png

@bnchrch bnchrch changed the title Test Connector Test Failure reported as success on no report Jun 15, 2023
@evantahler evantahler added the type/bug Something isn't working label Jun 20, 2023
@evantahler
Copy link
Contributor

Grooming:

If there's a dagger engine error (like the above), this is a failure. The logic to check if the report was good or not is likely needs to be inverted.

@bnchrch
Copy link
Contributor Author

bnchrch commented Jul 18, 2023

Grooming:

  • this is related to @marcosmarxm having a passing test report but failing unit tests: https://airbytehq-team.slack.com/archives/C03VDJ4FMJB/p1689628075772669
  • the pytest success/failure reporting is a bit brittle because it relies on stdout parsing and not exit code: if "failed" in last_log_line or "errors" in last_log_line: If the stdout structure changes we can have unexpected results.
  • This stdout parsing was originally introduced because Dagger raised errors without details when exit code > 0. We made pytest always exit with 0 and rely on stdout parsing to assess success or failure.
    This will be solved by upgrading to the latest dagger version, if a with_exec returns a non 0 exit code the error object now has stdout and stderr attribute.

Also likely a related issue: #27770

@bnchrch bnchrch changed the title Connector Test Failure reported as success on no report Connector Test Failure reported as success on failure Jul 18, 2023
@alafanechere
Copy link
Contributor

For the false positive with test results: we need to change how we parse CAT output and transform it to StepResult: replace the use of pytest_logs_to_step_result with get_step_result .
For the false positive without test result: to reproduce you should raise a an unhandled exception inside the pipeline, and I think the report wrongly states a success.

@bnchrch
Copy link
Contributor Author

bnchrch commented Jul 25, 2023

@alafanechere
Copy link
Contributor

For the false positive with test results: we need to change how we parse CAT output and transform it to StepResult: replace the use of pytest_logs_to_step_result with get_step_result .
For the false positive without test result: to reproduce you should raise a an unhandled exception inside the pipeline, and I think the report wrongly states a success.

This is addressed in #28767

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants