Bug 18488 – test_extractor misses version(unittest) blocks, causing `Deprecation: X is not visible from Y`

eg: occurred in https://github.com/dlang/phobos/pull/6178 ``` module bar; vesion(unittest){ void foo(){} } unittest{ foo; } ``` Deprecation: foo is not visible from bar The solution would be for test_extractor to export `vesion(unittest)` declarations. This would be easy to do if https://github.com/dlang-community/libdparse/issues/191 (Add precise source locations (start and end) to each ASTNode #191) was fixed (at least for declarations) the change would be something like: ``` override void visit(const Declaration decl){ auto u2=decl.conditionalDeclaration; if(!u2) return; auto u3=u2.compileCondition.versionCondition; if(!u3) return; auto token=u3.token; import dparse.lexer:tok; if(token.type!=tok!"unittest") return; foreach(ui;u2.trueDeclarations){ print(ui); // this would need to access start and end locations of decl } } ```

It's not a bug, but a feature. 1) ALL public examples need to be runnable locally and dlang.org That's not optional, but a deep requirement from mistakes in the past and the resulting bad image. 2) We don't use version(unittest) for future code and are about to weed out the last usages in Phobos, because you essentially just ended up with - accidentally exposing a public symbol - adding different behavior for -unittest (i.e. the testsuites) - there have been quite a few bugs where `version(unittest) { import std.stdio;}` led to bugs in user code because of templates and their dependence on the version(unittest) imports which obviously wasn't caught by the testsuite. While I really like your enthusiasm and in general a lot of things in the D lang are old, same are quite new and have a reason for being there, so sometimes taking a moment to step back and check why there's a CI warning (and not directly opening a bug report + finding "workarounds") would save you save time and frustration. > The solution would be for test_extractor to export `vesion(unittest)` declarations. That wouldn't help and you can already do better today with a ConditionalDeclaration visitor, e.g. in pseudo-code: --- override void visit(const ConditionalDeclaration decl){ // if decl is unittest auto text = cast(immutable(char)[]) sourceCode[decl.startLocation decl.endLocation]; } ---

> We don't use version(unittest) for future code and are about to weed out the last usages in Phobos this seems not true anymore, so `version(unittest)` is ok again, see: > FYI: that was before StdUnittest was reverted: #6202 https://github.com/dlang/phobos/pull/6178#discussion_r174675283

> this seems not true anymore, so `version(unittest)` is ok again, see: Well, it's an open problem - it was reverted because with -deps ALL unittest blocks are compiled (even the ones in Phobos). See also: https://github.com/dlang/phobos/pull/6202 https://github.com/dlang/phobos/pull/6159 Anyhow, as mentioned in my earlier comment we purposely don't include the version(unittest) symbols in the publicly documented tests as they aren't available for the user neither and wouldn't run on run.dlang.io or other places and we want to prevent such errors (people have rightfully complained a lot about examples from the docs not working locally in the past).

> people have rightfully complained a lot about examples from the docs not working locally in the past just curious, doesn't auto-tester make sure that the extracted documented unittests (produced by pipeline that extracts them from documented unittests) can be run (and don't generate failures) ?

(In reply to Timothee Cour from comment #4) > > people have rightfully complained a lot about examples from the docs not working locally in the past > > just curious, doesn't auto-tester make sure that the extracted documented > unittests (produced by pipeline that extracts them from documented > unittests) can be run (and don't generate failures) ? No the extraction is only run on CircleCi. This isn't a huge problem as it's almost always just missing imports or other accessibility issues. (I was talking about the past before this pipeline was introduced)

ok filed a bug for that so we don't forget: https://issues.dlang.org/show_bug.cgi?id=18619