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

[WIP] non regression tests for issue #16034 #16035

Closed

Conversation

smoothdeveloper
Copy link
Contributor

N.B. the current test infrastructure doesn't support evaluating FSX script that produces internal error, the added .bsl are pretending it can for now

@vzarytovskii if anyone in the team can help, it should be possible to check the output of FSI against .bsl file even when there is an internal exception being thrown.

@Happypig375 I've changed createBaselineErrors function you created, the property you access is already the "actual" / .err file, so it was creating .err.err; it also prints it is doing so.

Related: #16034

N.B. the current test infrastructure doesn't support evaluating FSX script that produces internal error, the added .bsl are pretending it can for now
@smoothdeveloper smoothdeveloper requested a review from a team as a code owner September 24, 2023 00:58
@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 24, 2023

Not entirely sure it's possible without some custom hosting and exception handling for it.

Or alternatively just launch fsi.dll and check its output, fsharpqa style

@smoothdeveloper
Copy link
Contributor Author

@vzarytovskii if it is not frowned upon to go shell out to fsi.dll, I'll revisit the bits of infrastructure to do this, albeit I'm not sure how to obtain the IL if doing so. I was trying to leverage the approach it is done in new tests.

I think having baselines for stderr, stdout, the IL is a must for many of the tests, we should really make it the default.

Also, nothing beats .fsx in terms of having advanced code, as you get the tooling, versus no tooling for standalone .fs or "code in string".

Feels we don't have enough tests for all overload / name resolution business going on, and the impediments to bring code under tests may be one reason.

@vzarytovskii
Copy link
Member

@vzarytovskii if it is not frowned upon to go shell out to fsi.dll, I'll revisit the bits of infrastructure to do this, albeit I'm not sure how to obtain the IL if doing so. I was trying to leverage the approach it is done in new tests.

I think having baselines for stderr, stdout, the IL is a must for many of the tests, we should really make it the default.

Also, nothing beats .fsx in terms of having advanced code, as you get the tooling, versus no tooling for standalone .fs or "code in string".

Feels we don't have enough tests for all overload / name resolution business going on, and the impediments to bring code under tests may be one reason.

Yeah, should be fine to do, we just need to make it obvious that it's running a separate process.

Do you want to move all fsi tests to a separate process? Or just selected ones? As for il - we use ildasm for current ones, not sure it will be easy for fsi ones. Technically emitted assembly can be intercepted, but not sure it will be trivial.

@smoothdeveloper
Copy link
Contributor Author

Do you want to move all fsi tests to a separate process? Or just selected ones?

I'm wary of moving / changing the infrastructure code for existing tests that I didn't add myself, so I'd do only for my own tests.

As for il - we use ildasm for current ones, not sure it will be easy for fsi ones. Technically emitted assembly can be intercepted, but not sure it will be trivial.

Maybe a separate run through FCS but it is a bit wasteful, assuming the infrastructure was put together to avoid running processes.

@smoothdeveloper smoothdeveloper changed the title non regression tests for issue #16034 [WIP] non regression tests for issue #16034 Sep 30, 2023
@smoothdeveloper
Copy link
Contributor Author

Closing this for now, I think it needs to be revisited one level higher in the stack, and taking care of the wrong logic regarding deprecation warning in https://github.com/dotnet/fsharp/blob/fb39b9440dd27f5ed093e0617cdce49d9dbffa3c/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/MemberDefinitions/NamedArguments/E_MustBePrefix.fs

I particularily don't like the current nested List.existsi which apply to almost every method application:

if not finalCalledMeth.IsIndexParamArraySetter &&
(finalCalledMeth.ArgSets |> List.existsi (fun i argSet -> argSet.UnnamedCalledArgs |> List.existsi (fun j ca -> ca.Position <> (i, j)))) then
errorR(Deprecated(FSComp.SR.tcUnnamedArgumentsDoNotFormPrefix(), mMethExpr))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants