Skip to content

Add expectBroken test for #7423 #8224

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 1 commit into from
Closed

Conversation

ffaf1
Copy link
Collaborator

@ffaf1 ffaf1 commented Jun 17, 2022

Test (expected fail) for #7423


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@ffaf1 ffaf1 force-pushed the cabal-check-tests branch 2 times, most recently from aed3a9d to 5f5c040 Compare June 17, 2022 15:25
@ffaf1
Copy link
Collaborator Author

ffaf1 commented Jun 17, 2022

Lovely individuals, this does not pass CI and I suspect the problem is related to

**How do I mark a test as broken?**  Use `expectBroken`, which takes
the ticket number as its first argument.  Note that this does NOT
handle accept-test brokenness, so you will have to add a manual
string output test, if that is how your test is "failing."

↑ I understand little of this. If you point me to a working example I can update the documentation to make it a bit clearer too!

@ffaf1 ffaf1 force-pushed the cabal-check-tests branch from 5f5c040 to d6c2724 Compare June 17, 2022 18:50
@ffaf1
Copy link
Collaborator Author

ffaf1 commented Jun 17, 2022

I have modified the test and now it should pass “expected fail” but I am still a bit confused:

  • what does not handling “accept-test brokenness” mean exactly? That test that should pass without a warning/error (when not broken) are not handled by expectBroken?
  • I have added a string output test as suggested. Is this correct? Is pointing that you can rewrite main in a simpler way once the test stops being broken correct?

(this was the offending line)

@ffaf1 ffaf1 marked this pull request as ready for review June 18, 2022 19:22
@fgaz
Copy link
Member

fgaz commented Jun 20, 2022

what does not handling “accept-test brokenness” mean exactly? That test that should pass without a warning/error (when not broken) are not handled by expectBroken?

I think it means that --accept won't write the expected correct output to the golden file because it can't know what it'll be, so you have to write it yourself (like you did)

I have added a string output test as suggested. Is this correct?

Looks correct

Is pointing that you can rewrite main in a simpler way once the test stops being broken correct?

why can't we write it like that already?

@ffaf1
Copy link
Collaborator Author

ffaf1 commented Jun 20, 2022

If I write it like this (cabal.test.hs):

main = cabalTest . expectBroken 7423 $ cabal "check" []

I get an:

UNEXPECTED OK

Hence I wonder what I am doing wrong.

@fgaz
Copy link
Member

fgaz commented Jun 20, 2022

Then I guess that's the meaning of this part:

so you will have to add a manual string output test, if that is how your test is "failing."

Looks like the expected output isn't checked at all with expectBroken, but that's the only way to tell if this test is failing since the -O2 warning isn't a fatal warning.

@fgaz
Copy link
Member

fgaz commented Jun 20, 2022

What about checking that the output does not contain O2, and keeping that even after #7423 is fixed?

@ffaf1 ffaf1 marked this pull request as draft June 20, 2022 12:55
@ffaf1 ffaf1 closed this Jun 22, 2022
@ffaf1 ffaf1 deleted the cabal-check-tests branch June 22, 2022 08:08
@Mikolaj
Copy link
Member

Mikolaj commented Jun 22, 2022

@ffaf1: just checking: are you exasperated and you rage-quit this PR :) or have you decided to follow a different design?

@ffaf1
Copy link
Collaborator Author

ffaf1 commented Jun 22, 2022

Lol no I have just renamed the branch to a more appropriate name!

Maybe renaming locally and deleting the remote was not the proper course of action!

I will see if I can fix this or I will just open a new PR as soon as I am finished with the one I am writing (i.e.: writing a testsuite for current cabal check capabilities).

@Mikolaj
Copy link
Member

Mikolaj commented Jun 22, 2022

No worries. :)

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