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

CI: convert tests to use Breeze tests command #23538

Closed
potiuk opened this issue May 6, 2022 · 12 comments · Fixed by #26612
Closed

CI: convert tests to use Breeze tests command #23538

potiuk opened this issue May 6, 2022 · 12 comments · Fixed by #26612
Assignees

Comments

@potiuk
Copy link
Member

potiuk commented May 6, 2022

The tests that are run in CI should use breeze and parallellism implemented in Python.

NOTE: We should pay an attention to provide better error feedback when docker containers could not be started - see #23523

@Bowrna
Copy link
Contributor

Bowrna commented May 7, 2022

@potiuk Could I pick up this issue and try?

@potiuk
Copy link
Member Author

potiuk commented May 7, 2022

Yeah. It's an ambitious task though :).

Just pay attention to the --parallell flag I've implemented in other commands (pull-images for one). What we've been doing so far - we run the tests in parallel for different test types when we had enough CPUs. But this was overly complex with gnu-parallel and we should do it as single breeze command with --parallel flag as parallelism is much better handled in Python and easier to reason about.

One of the goals of the task is not only to duplicate the current behaviour, but also improve it - using python we might want to do a bit better when it comes to grabbing the output of such parallel tests and displaying them to the user. The current solution "looks" good, but there were a few things that were just too complex to implement in Bash easily, but are entirely possible if we do it in Python.

There are a few properties for the target solution:

  • we need to see the progress of parallel tests (probably slightly different than the current approach as we can be a bit smarter in Python - from each parallel task we can see the progress of tests in % and just display the progress for each parallel task rather than what we did so far (showing last few lines from every parallell test type run)
  • We need to capture the output from each such parallel run and when all of them are complete we need to print them in separate folded CI group - marking the test types that failed in "red" and those that succeeded with "green". You can see at the current output of tests how it looks like it's pretty nice now.
  • Ideally we should also print a nice summary of only failed stuff after that so that the user sees immediately what failed without having to open the folded group (this shoudl be used if you want to see more details)
  • when there is an error we should also (this is currently done) grab the logs of containers and save them so that they can be uploaded as artifacts (this enormosly helps to diagnose problems).
  • also as mentioned in Cassandra container 3.0.26 fails to start on CI #23523 - when we fail to start docker-compose at all (for example when containers are unhealthy) we should make a better visual indication what's the actual error (marked in red)

The difficult thing is that it has to be "all in one go" - so it will be quite a big PR overall.

Also pay attention to a change I am about to submit - I am doing a refactor of packages/modules in Breeze - so you might want to wait with some heavy implementation once it is merged.

@Bowrna
Copy link
Contributor

Bowrna commented May 15, 2022

@potiuk This will include the changes in breeze tests command to add options for parallelism, run-in-parallel, python-versions etc. Am i right in understanding that part?

@potiuk
Copy link
Member Author

potiuk commented May 15, 2022

@potiuk This will include the changes in breeze tests command to add options for parallelism, run-in-parallel, python-versions etc. Am i right in understanding that part?

Correct.

@Bowrna
Copy link
Contributor

Bowrna commented May 15, 2022

Could you point me to any one of the tests in ci.yml file that has to be replaced with Breeze @potiuk ?

@potiuk
Copy link
Member Author

potiuk commented May 15, 2022

Could you point me to any one of the tests in ci.yml file that has to be replaced with Breeze @potiuk ?

Sure. Any test that is currently run by "ci_run_airflow_testing.sh":

https://github.com/apache/airflow/blob/main/.github/workflows/ci.yml#L1061

@Bowrna
Copy link
Contributor

Bowrna commented May 17, 2022

@potiuk Currently we check only if the versions are right and docker is running. We don't check in code if the container started is healthy. Do we need to check that before running command and error out in case if it is unhealthy?

Also, can you elaborate to me on which container you want to check? Here you have quoted about the Cassandra issue and the Cassandra container not starting.

@Bowrna
Copy link
Contributor

Bowrna commented May 17, 2022

  • We need to capture the output from each such parallel run and when all of them are complete we need to print them in separate folded CI group - marking the test types that failed in "red" and those that succeeded with "green". You can see at the current output of tests how it looks like it's pretty nice now.

@potiuk Can you point me to the place where the current output of tests is logged?

@Bowrna
Copy link
Contributor

Bowrna commented May 17, 2022

  • we need to see the progress of parallel tests (probably slightly different than the current approach as we can be a bit smarter in Python - from each parallel task we can see the progress of tests in % and just display the progress for each parallel task rather than what we did so far (showing last few lines from every parallell test type run)

Could i check by using tqdm to show the progress? @potiuk

@potiuk
Copy link
Member Author

potiuk commented May 18, 2022

@potiuk Currently we check only if the versions are right and docker is running. We don't check in code if the container started is healthy. Do we need to check that before running command and error out in case if it is unhealthy?

No need really. The health checks are done in Docker-compose already AND in the "entrypoint_ci.sh" when integrations are enabled. The cassandra case is the result of the first check. The main point there is to detect if the reason for failure was docker-compose health check failure (the first point) and print some better (red) error message in this case. You can simulate the case by modifying health check so that it will always fail ( for example here

)

@potiuk Can you point me to the place where the current output of tests is logged?

Few places. This is where it gets complicated (and we need to do it simpler);

This is complex and I think we can do simpler (and nicer) using python managing those parallel runs:

  1. no need to save output to files, we can just capture the output as usual and print them at the end from those captured variables. It will take a bit of memory, but it should be fine I think

  2. We do not need to print the last few lines reallly. We just do it to show the progress, but it is not THAT useful Rather than that we should simply do some kind of progress indicating how many lines of output are written for example for each job already and MAYBE in case of tests we should parse last few lines and try to guess "percent of progress" For example here: https://github.com/apache/airflow/runs/6498181414?check_suite_focus=true#step:10:240

  ### The last 2 lines for Providers process: /tmp/tmp.aRrLkYXmmw/tests/Providers/stdout ###
  tests/providers/amazon/aws/hooks/test_ec2.py .............               [  3%]
  tests/providers/amazon/aws/hooks/test_eks.py ...............

If we read last 90/100 characters we should be able to find the "[ 3%]" with regexp and simply print that :). With Python we can do a lot more easily than Bash.

  1. It's really useful to get the foldable outputs after all jobs are finished and color them green/red depending on status. Maybe also we can figure out some improvements there - but those could be done later.

Could i check by using tqdm to show the progress? @potiuk

Rich has fantastic customizable progress bar. https://rich.readthedocs.io/en/stable/progress.html. No need to use tqdm . However the problem is that progress bar in CI output is not working the way it works in terminal. We cannot really get such nice/dynamic progress bar there, we should think more about providing progress in percentage printed from time to time.

@Bowrna
Copy link
Contributor

Bowrna commented Jun 10, 2022

I think you have almost captured all the points you suggested to add in this PR Add CI-friendly progress output for tests

Can you tell me what are the other pending ones to finish in this PR @potiuk ?

@potiuk
Copy link
Member Author

potiuk commented Jun 10, 2022

Just parallell running - I have not covered that.

potiuk added a commit to potiuk/airflow that referenced this issue Oct 3, 2022
This is the last big change in "Rewrite Breeze to Python".

Fixes: apache#23538
potiuk added a commit that referenced this issue Oct 3, 2022
This is the last big change in "Rewrite Breeze to Python".

Fixes: #23538
ephraimbuddy pushed a commit that referenced this issue Nov 10, 2022
This is the last big change in "Rewrite Breeze to Python".

Fixes: #23538
(cherry picked from commit 2921796)
ephraimbuddy pushed a commit that referenced this issue Nov 10, 2022
This is the last big change in "Rewrite Breeze to Python".

Fixes: #23538
(cherry picked from commit 2921796)
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 a pull request may close this issue.

2 participants