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

Add CI-friendly progress output for tests #24236

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jun 5, 2022

This is the first step to run breeze tests in parallel in CI.
This flag adds "limited progress" output when running tests
which means that the runnig tests will just print few lines with
percent progress and color status indication from last few
progress lines of Pytest output, but when it completes, the whole output is
printed in a CI group - colored depending on status.

The final version (wnen we implement parallel test execution) should
also defer writing the output to until all tests are completed, but
this should be a follow-up PR.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragement file, named {pr_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member Author

potiuk commented Jun 5, 2022

cc: @Bowrna -> here is a part of the "parallel" test implementation. Not yet complete, but shoudl be closer to implement running tests in parallel and should be easy to parallelise it with #23715

@potiuk
Copy link
Member Author

potiuk commented Jun 5, 2022

Example output:

Screenshot 2022-06-06 at 00 24 21

  • Initially only the single lines are printed with "progress" (44%)
  • but when the whole tests complete, the output of tests is printed in the "foldable" group (In this case "Result of ...." )
  • in the output you see a "green" line, but in CI it will be a "folded" group of output (same as in the current tests)
  • the color of the group is "green" when tests succeed or "red" if they fail (same as in current tests)

@potiuk
Copy link
Member Author

potiuk commented Jun 5, 2022

Similar output for bigger number of tests when we see how the progress works:

Screenshot 2022-06-06 at 00 30 56

@potiuk potiuk force-pushed the add-limit-progress-output branch from 187e069 to 790ef69 Compare June 6, 2022 09:18
@potiuk
Copy link
Member Author

potiuk commented Jun 6, 2022

Last version has a bit better-suited output for Parallel CI tests. We see two lines of the output + overall progress for each test "process".

Screenshot from 2022-06-06 11-17-28

@potiuk potiuk force-pushed the add-limit-progress-output branch from 790ef69 to b31f3ff Compare June 6, 2022 09:25
@potiuk potiuk changed the title Add limited progress output for tests Add CI-friendly progress output for tests Jun 6, 2022
@potiuk potiuk force-pushed the add-limit-progress-output branch from b31f3ff to 03150a8 Compare June 6, 2022 09:51
@potiuk
Copy link
Member Author

potiuk commented Jun 6, 2022

Note - that this one is optimized for our CI usage. We cannot keep log output in memory, because we often might not have enough memory on GitHub Public runners. That's why each test "run" will simply write the output to a temporary file and we are using this file as the "progress status" source. By writing directly to file rather than getting the logs piped to our "managing" process we save precious memory for the actual test execution.

On the other hand, when we will run two tests in parallel, we cannot interleave the output from both processes directly to stdout because it will be a complete mess.

That's why it is also a bit more "complex" with separate thread monitoring the files and only occasionally prints the last few lines to show the progress (otherwise if someone watches the CI output they might be not aware that something is actually happening there and the tests are progresssing).

Seeing the "coloured" progress from the last two lines is also good so that there is an early indicator if there was an error already during test execution (the number will get red) so that the user can get early feedback and cancel/rerun CI build with a fix-up in case they are actively monitoring the output.

@potiuk
Copy link
Member Author

potiuk commented Jun 6, 2022

Tt's green and good next step to get rid of the bash finally

@Bowrna
Copy link
Contributor

Bowrna commented Jun 7, 2022

@potiuk I didn't make much progress in the issue #23538 as I was stuck on what I have to do next in the PR to proceed further. So I started focussing on other PR's and missed updating any progress on that issue.

Thanks for this PR. I will read this and start focussing on completing this issue.

@potiuk potiuk force-pushed the add-limit-progress-output branch from 03150a8 to b3d746f Compare June 7, 2022 09:46
@potiuk
Copy link
Member Author

potiuk commented Jun 7, 2022

@potiuk I didn't make much progress in the issue #23538 as I was stuck on what I have to do next in the PR to proceed further. So I started focussing on other PR's and missed updating any progress on that issue.

Thanks for this PR. I will read this and start focussing on completing this issue.

Yep. I just implemented it as I thought it might be useful for your PR (and got some fun while doing it :)

@potiuk potiuk force-pushed the add-limit-progress-output branch from b3d746f to 307a3ab Compare June 7, 2022 09:47
@potiuk
Copy link
Member Author

potiuk commented Jun 8, 2022

PR Looking for some approval here :)

@potiuk potiuk requested review from dstandish and josh-fell June 8, 2022 22:00
@potiuk
Copy link
Member Author

potiuk commented Jun 10, 2022

An approval would be nice :)

@potiuk potiuk requested a review from turbaszek June 10, 2022 22:25
@potiuk potiuk force-pushed the add-limit-progress-output branch from df7bdc6 to 89c6f3a Compare June 12, 2022 12:35
@potiuk potiuk force-pushed the add-limit-progress-output branch 3 times, most recently from ae18de9 to a9ba762 Compare June 13, 2022 22:10
@potiuk
Copy link
Member Author

potiuk commented Jun 13, 2022

Need it to improve testing speed in CI.

This is the first step to run breeze tests in parallel in CI.
This flag adds "limited progress" output when running tests
which means that the runnig tests will just print few lines with
percent progress and color status indication from last few
progress lines of Pytest output, but when it completes, the whole output is
printed in a CI group - colored depending on status.

The final version (wnen we implement parallel test execution) should
also defer writing the output to until all tests are completed, but
this should be a follow-up PR.
@potiuk potiuk force-pushed the add-limit-progress-output branch from a9ba762 to 436eb14 Compare June 14, 2022 14:23
Copy link
Contributor

@josh-fell josh-fell left a comment

Choose a reason for hiding this comment

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

Small comment on using the new Enum in other places, but otherwise LGTM.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jun 14, 2022
@github-actions
Copy link

The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk potiuk merged commit 41fefa1 into apache:main Jun 14, 2022
@potiuk potiuk deleted the add-limit-progress-output branch June 14, 2022 20:32
@potiuk potiuk mentioned this pull request Jun 14, 2022
potiuk added a commit to potiuk/airflow that referenced this pull request Jun 29, 2022
This is the first step to run breeze tests in parallel in CI.
This flag adds "limited progress" output when running tests
which means that the runnig tests will just print few lines with
percent progress and color status indication from last few
progress lines of Pytest output, but when it completes, the whole output is
printed in a CI group - colored depending on status.

The final version (wnen we implement parallel test execution) should
also defer writing the output to until all tests are completed, but
this should be a follow-up PR.

(cherry picked from commit 41fefa1)
@ephraimbuddy ephraimbuddy added this to the Airflow 2.3.3 milestone Jun 30, 2022
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants