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

test: remove broken marker, better define notimpl and notyet #9688

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

NickCrews
Copy link
Contributor

@NickCrews NickCrews commented Jul 24, 2024

inspired by #9535

The broken mark had the meaning of "this broke for some reason, but it wasn't the fault of this PR, so lets ignore it for now". I find this only meaningful in the context of that individual PR though: as soon as the PR is merged, the usefulness drops to near 0. I come across this mark as I'm reading code and I have no idea what action I should take. notimpl, notyet, and never are much more useful, they signal where the fault lies and what to do about it.

I went through and converted all the instances of broken to one of these other 3 marks. It was fairly fast and definitely error prone. I could see the argument for converting these all to plain xfails when we aren't sure. I'm not sure how bad it is for these marks to be incorrect.

I tweaked the prose that describes these 3 marks. I would love your editing of them.

@NickCrews NickCrews force-pushed the remove-mark-broken branch from 00b2e21 to 1904702 Compare July 27, 2024 08:11
@NickCrews NickCrews force-pushed the remove-mark-broken branch from 1904702 to cee1828 Compare July 28, 2024 05:46
Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

Thanks for putting this in @NickCrews -- I agree the utility of broken vanishes after a PR is merged.

At a glance, the marks you've chosen look reasonable - some of them may be wrong, but it's both subjective and imperfect, so I think we can just deal with it.

I think we should try this for a bit -- we may end up restoring something similar. To your point about "plain xfails", that may be the path we want to take (with whatever extra stuff we'll need to specify which backends to xfail for a given test).

@gforsyth gforsyth merged commit 20ceee5 into ibis-project:main Jul 29, 2024
97 checks passed
@cpcloud cpcloud added this to the 9.3 milestone Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants