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.

Sign up for GitHub

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

Jump to bottom

Fixed @test return value on Error (#31495) #36501

Merged
merged 3 commits into from
Jul 16, 2020
Merged

Fixed @test return value on Error (#31495) #36501

merged 3 commits into from
Jul 16, 2020

Conversation

tisztamo
Copy link
Contributor

@tisztamo tisztamo commented Jul 1, 2020 •
edited
Loading

This is directly mergeable and closes the bug #31495 by copying the two-liner proposed there by @tpapp , thus I do not feel comfortable prefixing RFC.

But this is also my first contribution as a boy scout and it would be great to get some feedback, especially because adding the missing tests was nontrivial and is not without issues:

  • I was only able to create the missing tests for record(::DefaultTestSet, ::T <: Result) methods, but not for @test itself.
  • Awkward output is printed, as seen here:

Screenshot from 2020-07-01 16-01-03

I think this is acceptable when a test system tests itself, but I would be happy to fix it. I just don't know how to start.

@tpapp
Copy link
Contributor

tpapp commented Jul 1, 2020

Thanks for fixing this and providing a test, but I am not sure why you renamed the function arguments in unrelated code, it will make them inconsistent with the rest of the file.

@tisztamo
Copy link
Contributor Author

tisztamo commented Jul 1, 2020 •
edited
Loading

Thanks for fixing this and providing a test, but I am not sure why you renamed the function arguments in unrelated code, it will make them inconsistent with the rest of the file.

I changed the names in methods of record() that I have found in this file, because they seemed to be inconsistent with the documentation:

https://github.com/JuliaLang/julia/blob/master/stdlib/Test/src/Test.jl#L669-L677

I failed to read other parts of the file, but now I see that t is used in quite a few places. As it always refers to a subtype of Result I still think that res would be a better name in every case, recovering file-level consistency. But I understand that this is subjective.

What do you think is the way to go? Should I rename them back to t, leave it as it is, change the documentation or change all occurrences of t to res?

@tpapp
Copy link
Contributor

tpapp commented Jul 2, 2020

Generally I would suggest going for minimal changes in a PR like this.

@tisztamo
Copy link
Contributor Author

tisztamo commented Jul 6, 2020

Reverted the renaming. Can I do anything else to improve this PR?

@tisztamo tisztamo changed the title Fixed @test return value on Error; Closes #31495 Fixed @test return value on Error (#31495) Jul 6, 2020
@tpapp
Copy link
Contributor

tpapp commented Jul 6, 2020

I think it is fine, you just have to wait for someone with commit rights to the repo to review it. Please give it some time, and ping here if it does not happen within a few days.

@tisztamo
Copy link
Contributor Author

bump

@StefanKarpinski StefanKarpinski merged commit 50325a8 into JuliaLang:master Jul 16, 2020
@StefanKarpinski
Copy link
Member

Thanks for the fix and the bump!

simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
@tisztamo @simeonschaub
Fixed @test return value on Error (JuliaLang#31495) (JuliaLang#36501)
7012d46 
Closes JuliaLang#31495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpapp tpapp tpapp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tisztamo @tpapp @StefanKarpinski