Skip to content

Conversation

@dtrifiro
Copy link
Contributor

@dtrifiro dtrifiro commented Jan 31, 2022

Due to changes in rich 11.1.0, all text formatted using rich.syntax.Syntax is now left-justified (see 0a37fcc),
resulting in output padded with whitespace to the size of the tty.

This resulted in broken tests due to mismatching whitespace between actual dvc.ui output and test cases.

Stripping whitespace in the ui output in test_exceptions solves the broken tests for both rich 11.1.0 and 11.0.0

fixes #7322

@dtrifiro dtrifiro requested a review from a team as a code owner January 31, 2022 20:46
@dtrifiro dtrifiro requested a review from pmrowla January 31, 2022 20:46
@dtrifiro dtrifiro force-pushed the fix/7322-test_strict_yaml-strip branch from 5d3b8b9 to 3b7060c Compare January 31, 2022 20:50
@efiop efiop requested review from skshetry and removed request for pmrowla January 31, 2022 21:17
Comment on lines 357 to 358
for l_expected, l_err in zip(expected.split("\n"), err.split("\n")):
assert l_expected.rstrip(" ") == l_err.rstrip(" ")
Copy link
Collaborator

@skshetry skshetry Feb 1, 2022

Choose a reason for hiding this comment

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

Let's use splitlines, and not use rstrip() in the expected value. Also rstrip defaults to ascii whitespace.

Suggested change
for l_expected, l_err in zip(expected.split("\n"), err.split("\n")):
assert l_expected.rstrip(" ") == l_err.rstrip(" ")
for l_expected, l_err in zip(expected.splitlines(), err.splitlines()):
assert l_expected == l_err.rstrip()

You may however need to fix some examples.

Copy link
Collaborator

@skshetry skshetry Feb 1, 2022

Choose a reason for hiding this comment

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

Few nitpicky suggestions:

  1. I prefer _line suffix than l_ prefix.
  2. Keep expected value on the right hand side. (That's fine I think).
Suggested change
for l_expected, l_err in zip(expected.split("\n"), err.split("\n")):
assert l_expected.rstrip(" ") == l_err.rstrip(" ")
for expected_line, err_line in zip(expected.splitlines(), err.splitlines()):
assert expected_line == err_line.rstrip()

But no strong opinion, just being nitpicky here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that using rstrip() instead of rstrip(" ") will also strip newlines

Copy link
Collaborator

@skshetry skshetry Feb 1, 2022

Choose a reason for hiding this comment

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

Note that using rstrip() instead of rstrip(" ") will also strip newlines

@dtrifiro, sorry, you are right. I was confusing it with bytes.rstrip() for some reason.

@dtrifiro dtrifiro force-pushed the fix/7322-test_strict_yaml-strip branch from 3b7060c to 3d3f9b9 Compare February 1, 2022 11:05
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
extra keys not allowed, in stages -> stage1 -> outs -> 0 -> logs ->\n\
extra keys not allowed, in stages -> stage1 -> outs -> 0 -> logs ->

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this fit on a single line now?

Suggested change
Found duplicate key "cmd" with value "python train.py" (original value:\
Found duplicate key "cmd" with value "python train.py" (original value: "python

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably linters will complain though, black does not for some reason, hmm.

Copy link
Collaborator

@skshetry skshetry Feb 1, 2022

Choose a reason for hiding this comment

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

Similarly, if python moves above, this could be written as:

Suggested change
"python\ntrain.py"), in line 4, column 5
train.py"), in line 4, column 5

@dtrifiro dtrifiro force-pushed the fix/7322-test_strict_yaml-strip branch from 3d3f9b9 to d506071 Compare February 1, 2022 11:21
@dtrifiro dtrifiro changed the title fix test_exceptions tests: fix test_exceptions Feb 1, 2022
@dtrifiro dtrifiro force-pushed the fix/7322-test_strict_yaml-strip branch from d506071 to a7d0aa4 Compare February 1, 2022 11:36
@dtrifiro dtrifiro merged commit 0562b4f into treeverse:main Feb 1, 2022
@dtrifiro dtrifiro deleted the fix/7322-test_strict_yaml-strip branch February 1, 2022 12:20
@skshetry
Copy link
Collaborator

skshetry commented Feb 1, 2022

Thank you so much @dtrifiro for the quickfix. 🙂

@efiop efiop added skip-changelog Skips changelog testing Related to the tests and the testing infrastructure labels Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Skips changelog testing Related to the tests and the testing infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tests: test_strict_yaml::test_exceptions failing on CI

3 participants