-
Notifications
You must be signed in to change notification settings - Fork 501
Adjust test outputs in test/examples to avoid warnings; fix bug in expandfirst handling on Windows
#2804
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
base: master
Are you sure you want to change the base?
Conversation
| - [`ccall`](@ref) | ||
| - [`deepcopy`](@ref) | ||
| - [`while`](@ref) | ||
| - [`@time(x)`](@ref) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explanation: ccall and @time docstrings reference other docstrings, and some of those have further ref's; that then leads to "missing reference" warnings, unless one pulls in all of those. So I replaced them with alternatives that (currently) don't reference anything in their docstrings
| ```jldoctest | ||
| julia> [1.0, 2.0, 3.0] | ||
| 3-element Array{Float64,1}: | ||
| 3-element Vector{Float64}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjust to changed output in... Julia 1.6 at least? Which is the minimum version supported by Documenter, so...
| ```jldoctest | ||
| julia> f(2) | ||
| ERROR: UndefVarError: f not defined | ||
| ERROR: UndefVarError: `f` not defined in `Main` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version of the message is shown in Julia >= 1.11; this is one of the reasons why we have warnonly = (VERSION.major == 1 && VERSION.minor <= 10) further above
| julia> fullname(@__MODULE__) | ||
| (:Main,) | ||
| (:Main, :Main) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure how this was ever correct... I mean, yeah, ideally it would print (:Main,) because that's what is printed in Main. But we are in a sandbox module, and we are replacing the module name in output by Main, so I think this is kinda the best we can hope for?
C.f.:
julia> fullname(@__MODULE__)
(:Main,)
julia> module Foo println(fullname(@__MODULE__)) end
(:Main, :Foo)
Main.Foo| ``` | ||
|
|
||
| # Issue398 | ||
| # Issue #398 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just for consistency (I am also tempted to sort these sections by issue number... or maybe each issue test should be in a file of its own?
| julia> f("") | ||
| ERROR: MethodError: no method matching f(::String) | ||
| The function `f` exists, but no method is defined for this combination of argument types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is new in Julia 1.11
| julia> println(Point) | ||
| Point | ||
| julia> import Base: sqrt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary to make this test pass in Julia >= 1.12
| # Bad links (Windows) | ||
|
|
||
| * [Colons not allowed on Windows -- `some:path`](some:path) | ||
| * [No "drive" -- `:path`](:path) | ||
| * [Absolute Windows paths -- `X:\some\path`](X:\some\path) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test caused a bunch of warnings. And it is unclear to me what it even is supposed to test. It was added in 80721fd from PR #485.
My best guess is in commit 80721fd which was part of PR #485 before squashing. It introduces a function fixlinks! which has some Windows specific code:
if is_windows() && ':' in img.url
Utilities.warn("Invalid local image: colons not allowed in paths on Windows\n '$(img.url)' in $(get(navnode.page))")
return
endThat function is long gone, but a descendant of the code lives on in local_links!:
elseif Sys.iswindows() && ':' in path
@docerror(
doc, :cross_references,
"invalid local link/image: colons not allowed in paths on Windows in $(Documenter.locrepr(page.source))",
link = node
)
return
endSo perhaps this line is about getting coverage for that @docerror
But despite this, I think it is the right move to remove this here: because right now, the test results in "fake coverage": we wouldn't notice if the test for colons was broken because we ignore the output via @quietly and even if we didn't, a failure here would most likely not trigger a CI failure.
So instead, we need a new test which triggers this and then specifically watches out for the result, and checks that it shows a warning/error with the appropriate message. (And that should probably be three tests, one for each instance).
As I mentioned elsewhere, I'd like such a setup also for testing the quality of the locrepr output, i.e. to ensure it shows the "right" file paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See PR #2822
| ```@eval | ||
| "expanded_"*"eval" | ||
| import Markdown | ||
| Markdown.parse("expanded_"*"eval") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this we get a warning because @eval should return nothing or a Markdown.MD. We don't want that here. Instead there should be a dedicated test for this warning, see issue #2805
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such a test is added in PR #2823
|
|
||
| ```@eval | ||
| rand(20, 20) | ||
| Markdown.MD("Hello") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Base.banner was moved to REPL.banner in Julia 1.11. But anyway, there seems no point in having a complex test here, is there?
| end | ||
| @test length(inv(":jl:keyword:`for`")) == 1 | ||
| if length(inv(":jl:keyword:`for`")) == 1 | ||
| item = inv[":jl:keyword:`for`"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to replace for by while because in recent Julia versions, the for docstring as a @ref to outer.
|
@mortenpi I tried to explain the reasoning behind all changes in PR review comments. But tests fail on Windows, any help would be appreciated: |
... so that we can run those tests with `warnonly = false`, at least in some specific Julia versions.
ed14185 to
4af527b
Compare
|
Ah of course it's right there in the error: My guess would be this is due to a different in path separators, combined with dumb string comparision on the paths. Indeed, the warning is produced here: function expand(doc::Documenter.Document)
priority_pages = filter(doc.user.expandfirst) do src
if src in keys(doc.blueprint.pages)
return true
else
@warn "$(src) in expandfirst does not exist"
return false
end
endI'll see if using But of course other kwargs for |
f88baf9 to
2a0b103
Compare
|
|
||
| * Modules for `@example` environments are now generated by `eval`'ing an expression, rather than invoking the `Module` constructor, which is not recommended. (#2683) | ||
| * Modules for `@example` environments are now generated by `eval`'ing an expression, rather than invoking the `Module` constructor, which is not recommended. ([#2683]) | ||
| * Fix handling of `expandfirst` entries containing slashes as path separators on Windows. ([#2804]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this uncovered a genuine bug in expandfirst handling on Windows, yay. I've thus added a changelog entry. (And fixed the link in the entry above)
expandfirst handling on Windows
1005449 to
80f2540
Compare
... so that we can run those tests with
warnonly = false, at least in some specific Julia versions.Resolves #2798.