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] Break partest pos check files [don't merge] #7243

Closed
wants to merge 1 commit into from

Conversation

NthPortal
Copy link
Contributor

I have been running into problems where partest doesn't verify the output for partest pos, so I'm checking if that's just a local problem or not.

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Sep 20, 2018
@SethTisue SethTisue added the WIP label Sep 20, 2018
@NthPortal NthPortal removed this from the 2.13.0-RC1 milestone Sep 20, 2018
@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Sep 20, 2018
@som-snytt
Copy link
Contributor

Good luck with that. It either compiles or it doesn't, it doesn't check output.

@SethTisue
Copy link
Member

SethTisue commented Sep 20, 2018

well but then what are all these?

~/scala.213 % ls test/files/pos/*.check
test/files/pos/constant-warning.check                     test/files/pos/t5692b.check
test/files/pos/delambdafy_t6260_method.check              test/files/pos/t7776.check
test/files/pos/macro-bundle-disambiguate-bundle.check     test/files/pos/t8001.check
test/files/pos/macro-bundle-disambiguate-nonbundle.check  test/files/pos/t8187.check
test/files/pos/macro-implicit-invalidate-on-error.check   test/files/pos/t8209a.check
test/files/pos/reflection-compat-api-universe.check       test/files/pos/t8209b.check
test/files/pos/reflection-compat-c.check                  test/files/pos/t8352.check
test/files/pos/reflection-compat-macro-universe.check     test/files/pos/t8364.check
test/files/pos/reflection-compat-ru.check                 test/files/pos/t8369a.check
test/files/pos/t10511.check                               test/files/pos/t8369b.check
test/files/pos/t5692a.check                               test/files/pos/t8719.check

@NthPortal
Copy link
Contributor Author

NthPortal commented Sep 20, 2018

the tests didn't fail

@SethTisue
Copy link
Member

is 2.12.x affected as well or is the problem 2.13 specific?

@NthPortal
Copy link
Contributor Author

@SethTisue it does appear to affect 2.12.x as well

@NthPortal
Copy link
Contributor Author

I assume it worked in #6410 when I added one of the tests, because I'm pretty sure that's how I generated the check file.

@NthPortal
Copy link
Contributor Author

another possibility is that those tests were originally somewhere else (e.g run) where it checked the output, and then were moved to pos afterward without realizing that it doesn't check the output.

@som-snytt
Copy link
Contributor

I haven't looked yet, but my deep expertise in partest says it doesn't consult a check file for pos tests, which is why it's necessary to use -Xfatal-warnings or whatever to fail on pos warnings. There may have been developments to which I was not privy because I'm not good enough to be an employee of scala center, so don't take my word for it.

I'd prefer a single test format where an annotation of some kind distinguishes "I must compile or not compile or run with this output." I promised to do that five years ago and didn't follow up, my bad. That would obviate moving tests between categories.

@NthPortal
Copy link
Contributor Author

(I'm closing this because its build results are available at this point, and it was never intended to be merged in any case. I don't mean to cut off discussion here (or on the linked issue) in any way.)

@NthPortal NthPortal closed this Sep 23, 2018
@som-snytt
Copy link
Contributor

For what it's worth, turning on check files for pos tests:

[error] Failed: Total 1561, Failed 356, Errors 0, Passed 1205

Even A.scala emits a warning without a check file.

Conceivably, one could use the check file if it exists, ignoring output otherwise. That must have been the intention of those zero-length check files.

@SethTisue
Copy link
Member

Conceivably, one could use the check file if it exists, ignoring output otherwise

that's what I was assuming

@SethTisue SethTisue removed this from the 2.13.0-RC1 milestone Sep 23, 2018
@som-snytt
Copy link
Contributor

I'll PR that to avoid this misunderstanding slash test black hole.

@NthPortal NthPortal deleted the partest-test branch October 15, 2018 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants