Skip to content

Delete super_editor.test #165

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

Merged
merged 1 commit into from
Oct 20, 2022
Merged

Delete super_editor.test #165

merged 1 commit into from
Oct 20, 2022

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Oct 19, 2022

These tests has never passed, and have historically suffered from superlistapp/super_editor#728

We should not have the run as part of the suite until they are passing.

@matthew-carroll FYI

These tests has never passed, and have historically suffered from superlistapp/super_editor#728

We should not have the run as part of the suite until they are passing.
@matthew-carroll
Copy link
Contributor

What do you mean they've never passed?

@dnfield
Copy link
Contributor Author

dnfield commented Oct 19, 2022

@matthew-carroll - I mean that the scripts always reported success, but the tests actually failed if you closely read the output. This is a frequent source of confusion now, since the scripts report success but the tests report failure.

I am fully supportive of adding these back,b ut we should add them back in a passing state.

@dnfield
Copy link
Contributor Author

dnfield commented Oct 20, 2022

Looking back, it seems like there was a time where this script was passing, so I misspoke :)

But it seems like they may have been failing for a long time. Because they did not report failure in a way detectable to CI, it's hard to tell when that started, and a frequent source of confusion. So for now we should just disable them and look to re-enable them separately.

@dnfield
Copy link
Contributor Author

dnfield commented Oct 20, 2022

Looks like they probably started failing with flutter/flutter#105407

@dnfield
Copy link
Contributor Author

dnfield commented Oct 20, 2022

The tests were added July 13th, the patch causing them to fail was merged August 5th. They have been silently failing for most of the time they've been around. It would be very difficult right now to go back and revert all of the patches that have further contributed to failures. My proposal is to instead patch super_editor forward and re-add it here.

@dnfield
Copy link
Contributor Author

dnfield commented Oct 20, 2022

And I'm deleting rather than just waiting for a fix forward because this has more than once caused confusion for gardeners trying to root cause unrelated failures - you see these errors when you go looking in the log and if you're not already familiar you have to track down why it's failing only to find out there's some other failure that's actually blocking the tree.

@auto-submit auto-submit bot merged commit a102c35 into main Oct 20, 2022
@dnfield dnfield deleted the dnfield-patch-1 branch October 20, 2022 00:43
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