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

Functional tests: allow grep_fail to accept options #6463

Open
wants to merge 1 commit into
base: 8.3.x
Choose a base branch
from

Conversation

MetRonnie
Copy link
Member

Like the description says. Split off from #6370

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • No dependency changes
  • No changelog/docs needed as internal
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@MetRonnie MetRonnie added the small label Nov 7, 2024
@MetRonnie MetRonnie added this to the 8.3.7 milestone Nov 7, 2024
@MetRonnie MetRonnie self-assigned this Nov 7, 2024
Comment on lines 34 to 37
# FIXME: this test doesn't work because of some interaction between the
# `script` command and colorama. The output includes a color reset char at
# the end despite using colorama.init(autoreset=False).
# script -q -c "cylc scan -t rich --color=never" log > /dev/null 2>&1
# grep_fail "$ANSI" log -P # no color
Copy link
Member

Choose a reason for hiding this comment

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

This test wasn't testing what it was supposed to test before?

Copy link
Member Author

Choose a reason for hiding this comment

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

No because the grep_fail function didn't use any of the options you provided

Copy link
Member

Choose a reason for hiding this comment

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

Ok, we should link an issue to this FIXME.

Would this close it: #6076 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No (I tried turning autoreset back on even though that wouldn't make any sense for it to fix the test, and it didn't help).


N.B. I had a look at how colorama works. It bills itself as a package meant only for making colour work on Windows, but I guess we are using it for its init(strip=not use_color) function that can automatically strip ANSI color chars from print() calls (it actually patches sys.stdout and sys.stderr). However there are >100 open issues and it is in a call-for-maintainers state: tartley/colorama#300.

Copy link
Member Author

Choose a reason for hiding this comment

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

Link to #6467 added

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Looks fine in terms of grep_fail options, question about the disabled test aside.

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 this pull request may close these issues.

3 participants