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

Fix Doctests for 0.5-dev #17106

Merged
merged 22 commits into from
Jul 12, 2016
Merged

Fix Doctests for 0.5-dev #17106

merged 22 commits into from
Jul 12, 2016

Conversation

omus
Copy link
Member

@omus omus commented Jun 25, 2016

Updates the doctests to be compliant with Julia 0.5-dev. This PR is based off of the work done in #15753.

Doctests currently have 14 remaining failures that can be categorized as:

Requires that JuliaLang/JuliaDoc#23 be merged. (DONE)

@vchuravy
Copy link
Member

Thanks for picking this up

@@ -5643,7 +5643,8 @@ julia> deleteat!([6, 5, 4, 3, 2, 1], 1:2:5)

julia> deleteat!([6, 5, 4, 3, 2, 1], (2, 2))
ERROR: ArgumentError: indices must be unique and sorted
in deleteat! at array.jl:543
in deleteat!(::Array{Int64,1}, ::Tuple{Int64,Int64}) at ./array.jl:571
in eval(::Module, ::Any) at ./boot.jl:231...
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as before, doesn't the doctest system already do some pruning of backtraces and wouldn't this be exactly the kind of thing to add to it?

Copy link
Member Author

@omus omus Jun 25, 2016

Choose a reason for hiding this comment

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

The doctest system was doing some pruning before but it seemed to only work for entire lines. The ellipsis works much better for us as sometimes we append extra information to the end of a line such as (repeats 2 times):

ERROR: ArgumentError: indices must be unique and sorted
 in deleteat!(::Array{Int64,1}, ::Tuple{Int64,Int64}) at ./array.jl:571
 in eval(::Module, ::Any) at ./boot.jl:231 (repeats 2 times)
 in _start() at ./client.jl:359

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense, but the eval(::Module, ::Any) part isn't really useful information - would it work if we put the ellipsis at the end of the previous line?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am ok with doing that change. I didn't do that as the original stack traces included eval(::Module, ::Any).

@tkelman just to confirm: You want me to removing the eval(::Module, ::Any) from all (almost all) stack traces in the manual?

Copy link
Contributor

@tkelman tkelman Jun 25, 2016

Choose a reason for hiding this comment

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

What do you mean by "original stack traces" ? Like what you get at the REPL or from running this code in a script (they'll look slightly different)? I think we already have the doctest system cutting out the pieces that appear in every single backtrace, 0.5 just adds a new such line. Since it's mostly noise I think it would be better not to add it. But would like to hear other opinions on the matter too.

Copy link
Member Author

@omus omus Jun 25, 2016

Choose a reason for hiding this comment

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

What do you mean by "original stack traces" ?

I meant what existed in the manual prior to this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Before this PR I'm seeing a handful, but quite a bit fewer than this PR is adding:

$ git grep -n 'in eval'
base/test.jl:914: in eval(::Module, ::Any) at ./boot.jl:226
doc/manual/mathematical-operations.rst:388:     in eval at ./boot.jl:263
doc/manual/mathematical-operations.rst:396:     in eval at ./boot.jl:263
doc/manual/mathematical-operations.rst:401:     in eval at ./boot.jl:263
doc/manual/mathematical-operations.rst:416:     in eval at ./boot.jl:263
doc/stdlib/test.rst:283:        in eval(::Module, ::Any) at ./boot.jl:226
test/core.jl:336:@test_throws ErrorException eval(:(abstract Qux2_{T} <: Sup2b_{Qux2_{Int}, T})) # wrapped in eval to avoid #16793

Copy link
Member Author

Choose a reason for hiding this comment

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

I stand corrected. It turns out that ce62f34 added in the eval line which I've been ignoring in my diffs. I'll remove those lines and probably most of the ellipsis as well.

@kshyatt kshyatt added test This change adds or pertains to unit tests docsystem The documentation building system labels Jun 25, 2016
@omus
Copy link
Member Author

omus commented Jun 25, 2016

Things are in a much better state now. I think this can be merged before the linked issues are dealt with as we can always fix the related doctests later.

@omus
Copy link
Member Author

omus commented Jun 30, 2016

Just waiting on #17202. I've left the doctests for #17107 broken as that issue requires that a section of the manual be re-written.

@omus
Copy link
Member Author

omus commented Jul 7, 2016

Rebased. Fixed some line numbers that were changed. One new doctest failure has appeared with "verbose_fussy_sqrt" reporting a line number of 0 in a stack trace #17317

@kshyatt
Copy link
Contributor

kshyatt commented Jul 7, 2016

Thanks for your patience and all your hard work on this, @omus.

@omus
Copy link
Member Author

omus commented Jul 8, 2016

No problem. I found the manual invaluable when learning Julia. Keeping things up to date is the least I can do.

@omus
Copy link
Member Author

omus commented Jul 8, 2016

Rebased again since #17202 has been merged. Doctests uncovered #17338 which make up 8/11 currently failing doctests. I additionally changed one of the doctests to be skipped (8683558) as it tended to give allot of false positives.

vchuravy and others added 11 commits July 11, 2016 08:22
cb8aa0b - fix most doctests in the manual that changed due to backtraces.
cd3797e - fix stdlib doctests
1892f72 - fix doctests in devdocs
d9495c7 - resolve Warning mismatch by renaming functions
83ef854 - convert superflous doctest to code-block
78f6d70 - Fix example about typemax and typemin

          Due to the changes in 4706184 tuples are now printed compact. Thus to
          demonstrate the point in this example we have to execute the examples
          independently.
c03e6e0 - fixup linenumbers
35a31bb - start->first

Ref JuliaLang#15753
The Stack Traces section of the manual was very outdated.
When ambiguous methods now report an error on usage rather than
definition: see JuliaLang#16125
omus added 11 commits July 11, 2016 08:22
Sphinx does not work well when docstring output contains an empty line.
Since `@code_warntype` has a empty line in the printed output this was
causing Sphinx to have incorrect expected data. As it turns out the
docstring example is only interested in the return type of the method
which meant we could truncate part of the output using ... and avoid the
empty line issue.
Corrections include:
- Remove addition of `eval(::Module, ::Any)` from most stacktraces
- Use doctest groups rather than appending numbers to function names
The `methods(+)` doctest seems to fail periodically due to the order
of the methods changing. It appears that blocks of these methods
typically stick together so I picked a nice reading block and attempted
to format with the least amount of ellipsis.

Here is an interesting failure which caused me to update the layout
of the doctest. Before this commit we used the following as a doctest:

```jldoctest
julia> methods(+)
 # 166 methods for generic function "+":
...
+(a::Float16, b::Float16) at float16.jl:136...
+(x::Float32, y::Float32) at float.jl:206...
+(x::Float64, y::Float64) at float.jl:207...
+(x::Bool, z::Complex{Bool}) at complex.jl:137...
+(x::Bool, y::Bool) at bool.jl:36...
+(x::Bool) at bool.jl:33...
+{T<:AbstractFloat}(x::Bool, y::T) at bool.jl:43...
+(x::Bool, z::Complex) at complex.jl:144...
+(x::Bool, A::AbstractArray{Bool,N<:Any}) at arraymath.jl:105...
+(x::Char, y::Integer) at char.jl:40
...
+(a, b, c, xs...) at operators.jl:119
```

The output was as follows:
```
 # 166 methods for generic function "+":
+(a::Float16, b::Float16) at float16.jl:136
+(x::Float32, y::Float32) at float.jl:206
+(x::Float64, y::Float64) at float.jl:207
+(x::Bool, z::Complex{Bool}) at complex.jl:137
+(x::Bool, y::Bool) at bool.jl:36
+(x::Bool) at bool.jl:33
+{T<:AbstractFloat}(x::Bool, y::T) at bool.jl:43
+(x::Bool, z::Complex) at complex.jl:144
+(x::Bool, A::AbstractArray{Bool,N<:Any}) at arraymath.jl:105
+(x::Char, y::Integer) at char.jl:40
...
+(a, b, c, xs...) at operators.jl:119
```
This particular doctest ended up producing a lot of false positives as
it appears the ordering from method can change from commit to commit.
Since this doctest was failing so consistently I set this particular
doctest to be skipped.

Additionally I expanded the sample of + methods shown which was
previously more restricted in an attempt to make this doctest pass more
consistently.
@omus
Copy link
Member Author

omus commented Jul 11, 2016

Another rebase since #17338 was fixed. Only three failed doctests remain. Two require the documentation to be updated (#17107) and the other should be addressed by #17324.

@@ -893,24 +893,19 @@ julia> typeof(f(1,2,3))
Int64

julia> @code_warntype f(1,2,3)
Variables:
Copy link
Contributor

Choose a reason for hiding this comment

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

is Variables output no longer being included in @code_warntype intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was intentional. I can add it back if it is preferred. The blank line was being an issue and I felt that the variable listing didn't add anything to the example.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh right I see the ... now, I guess it's alright to leave it out

@tkelman tkelman merged commit de39699 into JuliaLang:master Jul 12, 2016
@omus
Copy link
Member Author

omus commented Jul 12, 2016

Thanks for merging this @tkelman

@omus omus deleted the doctests branch July 12, 2016 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docsystem The documentation building system test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants