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

gh-97588: Failing tests to demonstrate the issue #97589

Closed
wants to merge 18 commits into from
Closed

gh-97588: Failing tests to demonstrate the issue #97589

wants to merge 18 commits into from

Conversation

matthiasgoergens
Copy link
Contributor

@matthiasgoergens matthiasgoergens commented Sep 27, 2022 •
edited
Loading

The tests are expected to fail, and are marked as such.

@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Sep 27, 2022
@matthiasgoergens matthiasgoergens changed the title gh-97588: Some tests to demonstrate the issue gh-97588: Failing tests to demonstrate the issue Sep 30, 2022
@matthiasgoergens
Copy link
Contributor Author

@mdickinson I marked the tests as failing, so we cant potentially get them into main as a basis for further work.

nanjekyejoannah
nanjekyejoannah reviewed Oct 6, 2022
View reviewed changes
Copy link
Contributor

@nanjekyejoannah nanjekyejoannah left a comment

Choose a reason for hiding this comment

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

Shouldn't we be patching a fix instead? Unless we are accepting this as desired behaviour.

@matthiasgoergens
Copy link
Contributor Author

@nanjekyejoannah See #97702 for the fix.

nanjekyejoannah
nanjekyejoannah reviewed Oct 7, 2022
View reviewed changes
Copy link
Contributor

@nanjekyejoannah nanjekyejoannah left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Then we close this to merge the fix instead, no?

@matthiasgoergens
Copy link
Contributor Author

@nanjekyejoannah I'm not completely confident in the fix, yet. But I am confident that these test cases are good.

Not sure we have a general policy against contributing failing test cases?

nanjekyejoannah
nanjekyejoannah reviewed Oct 11, 2022
View reviewed changes
Copy link
Contributor

@nanjekyejoannah nanjekyejoannah left a comment

Choose a reason for hiding this comment

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

The tests test the failing caes. But with the fix, these tests will change i.e not failing, I would prefer reviewing a PR with a fix and updated tests, since there is a fix. What do you think?

@matthiasgoergens
Copy link
Contributor Author

@nanjekyejoannah Feel free to review #97702

@bedevere-bot bedevere-bot mentioned this pull request Mar 14, 2023
Closed
@encukou
Copy link
Member

encukou commented Aug 26, 2024

These were merged with #97702. Thank you!

@encukou encukou closed this Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nanjekyejoannah nanjekyejoannah nanjekyejoannah left review comments

Assignees
No one assigned
Labels
awaiting review tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matthiasgoergens @encukou @nanjekyejoannah @bedevere-bot