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 item seems to fail only in VS Code #38

Open
nickrobinson251 opened this issue Jan 12, 2023 · 7 comments
Open

Test item seems to fail only in VS Code #38

nickrobinson251 opened this issue Jan 12, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@nickrobinson251
Copy link
Contributor

I'm using TestItems and VS Code for the first time, so high chance of user error, but...

I tried changing a package over to use TestItems.jl (PR), and the tests pass when run on CI and when run locally in the REPL via ] test, but fail in VSCode

The test which fails (only in VS Code) is

 Expression: sprint(show, s) == "String7(\"abc\")"
 Evaluated: "InlineStrings.String7(\"abc\")" == "String7(\"abc\")"`

which looks like a scoping issue?!

Code here: https://github.com/JuliaStrings/InlineStrings.jl/pull/62/files#diff-3b9314a6f9f2d7eec1d0ef69fa76cfabafdbe6d0df923768f9ec32f27a249c63L551-L560

(Also there seems to be an off-by-one error in the reporting of the failed test line number, see screenshot below)

Screenshot 2023-01-12 at 17 25 21


p.s. let me know if there's a better place for this kind of issue

@davidanthoff
Copy link
Member

Hm, this must somehow depend on the show method for types, right? Is https://github.com/JuliaStrings/InlineStrings.jl/blob/9b4ff8ff67dc0441b58383becba970370a30c1d1/src/InlineStrings.jl#L179 the line where the different output originates, right? Do you know under what circumstance that show method is adding the module name and when it doesn't do that?

@davidanthoff davidanthoff added the bug Something isn't working label Jan 12, 2023
@quinnj
Copy link
Contributor

quinnj commented Jan 12, 2023

I think I remember @nalimilan being involved in this for Base Julia; i.e. it decides whether the print the module name based on what is currently loaded/reachable in the current scope? So if a submodule is loaded, then it won't print the parent module prefix anymore because the submodule itself is reachable? Something along those lines.

@davidanthoff
Copy link
Member

The relevant pieces of code are https://github.com/julia-vscode/TestItemRunner.jl/blob/main/src/TestItemRunner.jl#L62 and https://github.com/julia-vscode/julia-vscode/blob/main/scripts/packages/VSCodeTestServer/src/VSCodeTestServer.jl#L56. From just staring at it, I can't really see a difference that would explain this... In both cases we create a new module inside of Main, and then in both cases we either load (or don't) the package that is to be tested via using, and then execute the actual test code inside that module...

@davidanthoff
Copy link
Member

(Also there seems to be an off-by-one error in the reporting of the failed test line number, see screenshot below)

Hm, weird, that doesn't reproduce on my system:

image

@davidanthoff
Copy link
Member

Actually, I know what is happening here :)

https://github.com/JuliaStrings/InlineStrings.jl/pull/62/files#diff-3b9314a6f9f2d7eec1d0ef69fa76cfabafdbe6d0df923768f9ec32f27a249c63R2 is the reason there is a difference. That line loads the InlineStrings package into the Main module. When that is the case, the type printing will realize that InlineStrings is reachable from Main and not display the module name for the type. When run in the VS Code extension, though, InlineStrings is not loaded into Main, and so when the show code checks whether that name is reachable from Main, it isn't and it will print the module and type name. So the short-term fix would be to just remove this using InlineStrings line, it doesn't do anything in any case.

For a moment I thought we could actually store the temporary module where test code is executed in the IOContext :module property so that one can always avoid the module name (see https://github.com/JuliaLang/julia/blob/d61cfd253b95993ef0dfa1a52cee0997a07b9d46/base/show.jl#L720 for how that is picked up), but I think what would then happen is that all other types might change their printing? Probably worth experimenting with.

@nickrobinson251
Copy link
Contributor Author

That line loads the InlineStrings package into the Main module. When that is the case, the type printing will realize that InlineStrings is reachable from Main and not display the module name for the type. When run in the VS Code extension, though, InlineStrings is not loaded into Main, and so when the show code checks whether that name is reachable from Main, it isn't and it will print the module and type name.

Yeah, that all sounds right.

So the short-term fix would be to just remove this using InlineStrings line, it doesn't do anything in any case.

Well, that would fix the descrepency, by also making this test fail outside of VS Code... but it doesn't fix the issue which is this test should pass in VS Code. Otherwise, there's no way to test that (exported) types print the way we expect (i.e. without module qualification in environments where the module is loaded).

@davidanthoff
Copy link
Member

I think this is kind of a hard corner case, because the behavior of the show method from base simply is so different depending on the context from which one is running it. I don't see a way to run the test item code in the Main module because then we can't reload that... I think in Julia 1.9 (or something like that) there is a notion of an "active" module, and the show method might check whether the name is reachable from that. So we might be able to use that here, i.e. make the temporary module where the test code is executing the active module, but that won't work on older Julia versions...

I think at the end of the day I would probably just change the test here, so that it includes the module name? And if you remove the using from the runtests.jl file, then it should pass in both cases? Yes, you are then no longer testing the show method that doesn't include the module name, but really that is base functionality in any case? And maybe OK to not test that in InlineStrings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants