-
Notifications
You must be signed in to change notification settings - Fork 783
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
Consolidate Compiler ErrorMessage tests under one suite. #9525
Conversation
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.
Minor tweaks, then it's good to go , thanks
tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj
Show resolved
Hide resolved
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 I've added
fsharp/tests/FSharp.Test.Utilities/CompilerAssert.fs Lines 449 to 478 in d53716c
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). |
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. |
tests/FSharp.Compiler.ComponentTests/ErrorMessages/InvalidNumericLiteralTests.fs
Outdated
Show resolved
Hide resolved
…ricLiteralTests.fs Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
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. |
@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. |
* 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>
As part of #7075