Skip to content

[6.0] [Yaml] Restore two failing test methods in DumperTest. #47168

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

Closed
wants to merge 2 commits into from

Conversation

donquixote
Copy link
Contributor

@donquixote donquixote commented Aug 3, 2022

The methods were lost in merge commit 8752c8b - too bad we can't show a direct diff between two commits for a single file in github.

The test methods document the misbehavior as known issues. They will be changed when the failing behavior is fixed in #46731.
(I personally think this is a good way to write tests in preparation for a fix)

Unlike in 4.4, in 6.0 these failing calls throw exceptions.

This should be a preparation for #46731, or before merging #47167 to 6.0.

Q A
Branch? 6.0
Bug fix? no
New feature? no
Deprecations? no
Tickets Follow-up to PR #46739
License MIT
Doc PR

@donquixote donquixote requested a review from xabbuh as a code owner August 3, 2022 00:32
@carsonbot carsonbot added this to the 6.0 milestone Aug 3, 2022
@donquixote donquixote changed the title [Yaml] Restore two failing test methods in DumperTest. [6.0] [Yaml] Restore two failing test methods in DumperTest. Aug 3, 2022
Unlike in 4.4, in 6.0 these failing calls throw exceptions.
@nicolas-grekas
Copy link
Member

We'd better merge the PR fixing these issues rather than merging "expected failures".
That would better align with our processes.
No hurry, please submit when you can.
Closing meanwhile.

@donquixote
Copy link
Contributor Author

We'd better merge the PR fixing these issues rather than merging "expected failures".
That would better align with our processes.

I like a strategy where we document known issues as tests with @todo or some other marker.
So we would have a "known issue" in 6.0, but a fix in 6.2.
But I am aware this is not the common way of doing things. So ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants