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

Mark failing tests as expected failures, or fix them #8022

Merged
merged 6 commits into from
Mar 4, 2022

Conversation

robx
Copy link
Collaborator

@robx robx commented Feb 28, 2022

This marks tests that are currently failing on CI as "expected broken", to move back towards "green".

My understanding is that these were broken all along, but only started visibly failing recently with an extended test matrix through #7952.

Also


Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • no changelog, since no user-visible changes
  • no documentation update necessary

@jneira
Copy link
Member

jneira commented Feb 28, 2022

I dont like see commits and prs with red crosses neither. It has a point though: make us eager to fix failing tests.
Skipping them is somewhat like hiding it and then the fix is delayed until the end of the times.
Is ther a way to make them broken, so at least they will fail if they are fixed? I think i've seen messages similar to "expected broken" in test logs.
That way we can avoid skipped tests which should not be, like https://github.com/haskell/cabal/pull/8022/files#diff-33a6bc12843418321803ea88d60a1bc8f76f352619a287ae4b91e1c299e77b87L8

@jneira
Copy link
Member

jneira commented Feb 28, 2022

Also, could be a comment be attached mentioning the issue?

@robx
Copy link
Collaborator Author

robx commented Feb 28, 2022

I mean, when we get a bug report, we'd never first merge a failing test either while the fix is (maybe or not) incoming sooner or later, right?

Expected failures would be nice, but they're not available at this point. I'm happy to file an issue so that can be done as a follow-up.

And yeah I absolutely mean to relevant tickets, meant to already, will fix if I didn't.

@jneira
Copy link
Member

jneira commented Feb 28, 2022

And yeah I absolutely mean to relevant tickets, meant to already, will fix if I didn't.

I mean mentioning it in the test itself, to be shown in test logs, if possible

@andreabedini
Copy link
Collaborator

As long as we capture the information and link to the relevant tickets (which @Mikolaj has done), I am ok with this.

@jneira
Copy link
Member

jneira commented Mar 1, 2022

Expected failures would be nice, but they're not available at this point. I'm happy to file an issue so that can be done as a follow-up

could we not use

expectBrokenIf :: Bool -> Int -> TestM a -> TestM ()

?

It has the ticket and can be guarded with os and ghc version iiuic

@jneira
Copy link
Member

jneira commented Mar 1, 2022

This will have the nice effect of removing experimental from the linux/macos workflow and the matrix will be lot simpler

@andreabedini
Copy link
Collaborator

IMHO using expectBrokenIf would be a clever solution since it declares our expectations more clearly than just skipping the test. 👍 from me.

@robx robx force-pushed the skip branch 2 times, most recently from 8a4c349 to 611f8be Compare March 3, 2022 21:32
@robx robx marked this pull request as ready for review March 3, 2022 23:27
@robx robx changed the title Skip failing tests Mark failing tests as expected failures, or fix them Mar 3, 2022
robx added 2 commits March 4, 2022 08:08
There appears to be a small regression in heap usage with GHC 9.2.1.
This raises the limit to accommodate that, and give a bit of a buffer.
Compare analysis at haskell#8029.
@jneira
Copy link
Member

jneira commented Mar 4, 2022

Great work, many thanks, some reason to not handle PackageTests/NewBuild/T3827?

@jneira jneira closed this Mar 4, 2022
@jneira
Copy link
Member

jneira commented Mar 4, 2022

oops

@jneira jneira reopened this Mar 4, 2022
@robx
Copy link
Collaborator Author

robx commented Mar 4, 2022

Great work, many thanks, some reason to not handle PackageTests/NewBuild/T3827?

You're welcome!

I just pushed an amended commit to handle T3827 more generally (previously I had skipped just the macos 8.10.7 failure).

@@ -3,7 +3,10 @@ import Test.Cabal.Prelude
-- an executable RPATH. Don't test on Windows, which doesn't
-- support RPATH.
main = setupAndCabalTest $ do
skipIfWindows
skipIfWindows
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could move this skipIfWindows to expectBrokenIf ((osx || win) && ghc)

maybe it should be in other pr, revising all skipIfWindows and transform as much of them as possible in knownBrokenIfWindows

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think I'd prefer to leave this out. Skipping might also be more appropriate (in some of those ifWindows cases at least), where we test non-Windows functionality, instead of something that we'd usually expect to work but that's broken for some reason.

(There's a second difference -- expectBrokenIf tests will be run, while skipIf won't. So e.g. for tests that are too slow, we can't use expectBrokenIf.)

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

Many thanks again, eager to see green checks everywhere again 😺

@jneira jneira requested review from Mikolaj and andreabedini March 4, 2022 09:56
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@jneira jneira merged commit c2eb7ba into haskell:master Mar 4, 2022
@jneira
Copy link
Member

jneira commented Mar 4, 2022

Ok we can add all jobs to master protection rules!

@ulysses4ever ulysses4ever mentioned this pull request Jun 25, 2022
3 tasks
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.

4 participants