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

Consolidate Compiler ErrorMessage tests under one suite. #9525

Merged
merged 14 commits into from
Jun 23, 2020

Conversation

vzarytovskii
Copy link
Member

As part of #7075

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Minor tweaks, then it's good to go , thanks

@smoothdeveloper
Copy link
Contributor

In my experience, tests asserting error messages are easier to maintain when they use the baseline files.

Adjusting the test code when they are using CompilerAssert.CompileWithErrors is more tedious than running a diff on .err and .bsl files. It is also true for authoring new tests, and making sure the test covers all the compiler output, not just cherry picking on subset of messages (which can still hide potential regressions).

I've added TypeCheckWithErrorsAndOptionsAgainstBaseLine to make it convenient to use the baseline system under the new testing infrastructure, it may be worth for new tests asserting on error messages to use this system instead:

  • tests/FSharp.Compiler.ComponentTests/ErrorMessages/ConfusingTypeName.fs
  • tests/FSharp.Compiler.ComponentTests/ErrorMessages/AccessOfTypeAbbreviationTests.fs

static member TypeCheckWithErrorsAndOptionsAgainstBaseLine options (sourceDirectory:string) (sourceFile: string) =
lock gate <| fun () ->
let absoluteSourceFile = System.IO.Path.Combine(sourceDirectory, sourceFile)
let parseResults, fileAnswer =
checker.ParseAndCheckFileInProject(
sourceFile,
0,
SourceText.ofString (File.ReadAllText absoluteSourceFile),
{ defaultProjectOptions with OtherOptions = Array.append options defaultProjectOptions.OtherOptions; SourceFiles = [|sourceFile|] })
|> Async.RunSynchronously
Assert.IsEmpty(parseResults.Errors, sprintf "Parse errors: %A" parseResults.Errors)
match fileAnswer with
| FSharpCheckFileAnswer.Aborted _ -> Assert.Fail("Type Checker Aborted")
| FSharpCheckFileAnswer.Succeeded(typeCheckResults) ->
let errorsExpectedBaseLine =
let bslFile = Path.ChangeExtension(absoluteSourceFile, "bsl")
if not (File.Exists bslFile) then
// new test likely initialized, create empty baseline file
File.WriteAllText(bslFile, "")
File.ReadAllText(Path.ChangeExtension(absoluteSourceFile, "bsl"))
let errorsActual =
typeCheckResults.Errors
|> Array.map (sprintf "%A")
|> String.concat "\n"
File.WriteAllText(Path.ChangeExtension(absoluteSourceFile,"err"), errorsActual)
Assert.AreEqual(errorsExpectedBaseLine.Replace("\r\n","\n"), errorsActual.Replace("\r\n","\n"))

This would also lend well into something where we cover all actual error messages the compiler can produce (which isn't the case, we've seen a regression on match on DU cases), which can also be useful for future work on compiler documentation (description of each error message with samples and explanation to fix).

@baronfel
Copy link
Member

regarding the last point about compiler error message documentation I 100% took samples and phrasing from the baseline tests because they were such an easy to read resource.

…ricLiteralTests.fs

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
@vzarytovskii
Copy link
Member Author

@smoothdeveloper

In my experience, tests asserting error messages are easier to maintain when they use the baseline files.

Adjusting the test code when they are using CompilerAssert.CompileWithErrors is more tedious than running a diff on .err and .bsl files. It is also true for authoring new tests, and making sure the test covers all the compiler output, not just cherry picking on subset of messages (which can still hide potential regressions).

I've added TypeCheckWithErrorsAndOptionsAgainstBaseLine to make it convenient to use the baseline system under the new testing infrastructure, it may be worth for new tests asserting on error messages to use this system instead:

  • tests/FSharp.Compiler.ComponentTests/ErrorMessages/ConfusingTypeName.fs
  • tests/FSharp.Compiler.ComponentTests/ErrorMessages/AccessOfTypeAbbreviationTests.fs

It makes sense to me, I wouldn't do it as a part of this PR thought, this one is just moving tests from older suites to a new one.
I will make it as a separate though.

@vzarytovskii vzarytovskii merged commit 3a067f4 into dotnet:master Jun 23, 2020
@vzarytovskii vzarytovskii deleted the move-compiler-tests branch June 23, 2020 17:47
@smoothdeveloper
Copy link
Contributor

@vzarytovskii sorry I didn't catch the tests I mentioned were just moved.

What I say makes most sense for new tests around error / warnings, or at the time change to the actual message makes the test red.

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* Consolidate ErrorMessages compiler tests under component tests suite
* Fix newlines in the assertion
* Cleaned up test list; Make RIDs conditional as well

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants