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.

Sign up for GitHub

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

Jump to bottom

trunner: prevent the screen from clear by escape code in output from fail #358

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

trunner: prevent the screen from clear by escape code in output from fail #358

wants to merge 1 commit into from

Conversation

mateusz-bloch
Copy link
Member

JIRA: CI-460

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

  • Already covered by automatic testing.
  • New test added: (add PR link here).
  • Tested by hand on: (list targets here).

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing linter checks and tests passed.
  • My changes generate no new compilation warnings for any of the targets.

Special treatment

  • This PR needs additional PRs to work (list the PRs, preferably in merge-order).
  • I will merge this PR by myself when appropriate.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 12, 2024 •
edited
Loading

Unit Test Results

7 484 tests   - 241   6 769 ✅  - 241   38m 21s ⏱️ + 1m 17s
  420 suites  -  25     715 💤 ±  0 
    1 files   ±  0       0 ❌ ±  0 

Results for commit 933c44c. ± Comparison against base commit aba8104.

This pull request removes 241 tests. 
phoenix-rtos-tests/mem/mmap ‑ armv7a7-imx6ull-evk:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned
phoenix-rtos-tests/mem/mmap ‑ armv7a7-imx6ull-evk:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned_file
phoenix-rtos-tests/mem/mmap ‑ armv7a7-imx6ull-evk:phoenix-rtos-tests/mem/mmap.test_mmap.len__zero
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-qemu:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-qemu:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned_file
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-qemu:phoenix-rtos-tests/mem/mmap.test_mmap.len__zero
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-zedboard:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-zedboard:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned_file
phoenix-rtos-tests/mem/mmap ‑ armv7a9-zynq7000-zedboard:phoenix-rtos-tests/mem/mmap.test_mmap.len__zero
phoenix-rtos-tests/mem/mmap ‑ armv7m4-stm32l4x6-nucleo:phoenix-rtos-tests/mem/mmap.test_mmap.len__not_page_aligned
…

♻️ This comment has been updated with latest results.

@mateusz-bloch mateusz-bloch force-pushed the mateusz-bloch/avoid_printing_escape_codes_for_fail branch from c49e59c to a749936 Compare June 14, 2024 10:25
@mateusz-bloch mateusz-bloch marked this pull request as ready for review June 14, 2024 13:56
@nalajcie
Copy link
Member

Why? IMHO on error we should show data as-is (even garbage) as it might be a part of (or a reason for) an error?

The regex is slightly invalid, also we already have remove_ansi_sequences function available.

@damianloew
Copy link
Contributor

Why? IMHO on error we should show data as-is (even garbage) as it might be a part of (or a reason for) an error?

The regex is slightly invalid, also we already have remove_ansi_sequences function available.

Why? IMHO on error we should show data as-is (even garbage) as it might be a part of (or a reason for) an error?

The regex is slightly invalid, also we already have remove_ansi_sequences function available.

The name of task suggested that we could skip clear screen esc code - I didn't mean to escape all of them. I was considering such solution, because we are running tests using trunner on images that refresh, clear and print the whole output once again and again. Then in case of fail the output is not readable

@mateusz-bloch mateusz-bloch marked this pull request as draft June 17, 2024 10:06
@mateusz-bloch mateusz-bloch force-pushed the mateusz-bloch/avoid_printing_escape_codes_for_fail branch from a749936 to efc7483 Compare June 17, 2024 15:57
@mateusz-bloch mateusz-bloch changed the title trunner: prevent printing escape codes in output from fail trunner: prevent clear screen by escape codes in output from fail Jun 17, 2024
@mateusz-bloch mateusz-bloch changed the title trunner: prevent clear screen by escape codes in output from fail trunner: prevent the screen from clear by escape code in output from fail Jun 17, 2024
@mateusz-bloch mateusz-bloch force-pushed the mateusz-bloch/avoid_printing_escape_codes_for_fail branch from efc7483 to e4c83fe Compare June 17, 2024 16:00
@mateusz-bloch mateusz-bloch marked this pull request as ready for review June 17, 2024 16:00
@damianloew damianloew force-pushed the mateusz-bloch/avoid_printing_escape_codes_for_fail branch from e4c83fe to c5c82f6 Compare October 10, 2024 09:00
damianloew
damianloew requested changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@damianloew damianloew left a comment

Choose a reason for hiding this comment

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

About the commit message: That's very clear, but a bit too long, let's make it shorter. You can add a quick explanation in commit description why are we adding this.

trunner/types.py Outdated Show resolved Hide resolved
trunner/types.py Outdated Show resolved Hide resolved
@mateusz-bloch
trunner: prevent the screen clear in failure output
933c44c 
Ensures that escape codes in output don't clear the screen when a failure occurs.

JIRA: CI-460
@mateusz-bloch mateusz-bloch force-pushed the mateusz-bloch/avoid_printing_escape_codes_for_fail branch from c5c82f6 to 933c44c Compare October 10, 2024 09:40
@nalajcie
Copy link
Member

The name of task suggested that we could skip clear screen esc code - I didn't mean to escape all of them. I was considering such solution, because we are running tests using trunner on images that refresh, clear and print the whole output once again and again. Then in case of fail the output is not readable

I still don't get it, the log should be as verbose as it could (if we're removing data from the output we might remove why regex match has failed) . If we're talking about better presentation on screen - it should be done at a different level (just before printing message to screen)?

Also please note that current implementation is slow - using simple replace twice would probably be faster. If we would have multiple things to remove - the least you could do is compile the regex beforehand.

@mateusz-bloch mateusz-bloch marked this pull request as draft October 10, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damianloew damianloew damianloew requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mateusz-bloch @nalajcie @damianloew