-
Notifications
You must be signed in to change notification settings - Fork 831
Move signature tests from cambridge suite to component tests #14317
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
Conversation
psfinaki
left a comment
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.
Good stuff, great job. Put a few small remarks, of course, there is probably a lot that can be improved (e.g. inconsistent casing) but this is not the point of the PR.
Food for thought - I'm currently splitting Editor.Tests into a separate project, have you considered splitting this into a separate project as well? "Component tests" are quite bloated, to the level the name of the project doesn't tell much anymore.
tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj
Outdated
Show resolved
Hide resolved
....Compiler.ComponentTests/Signatures/TestCasesForGenerationRoundTrip/access-minimal-repro.fsx
Show resolved
Hide resolved
....Compiler.ComponentTests/Signatures/TestCasesForGenerationRoundTrip/access-minimal-repro.fsx
Outdated
Show resolved
Hide resolved
tests/FSharp.Compiler.ComponentTests/Signatures/TestCasesForGenerationRoundTrip/access.fsx
Show resolved
Hide resolved
tests/FSharp.Compiler.ComponentTests/Signatures/TestCasesForGenerationRoundTrip/array.fsx
Show resolved
Hide resolved
Personal opinion - For a very independent set of features, say "Tests for VS tooling", it might make sense if all existing tests are moved to a single place as well (even if just copy pasted using their existing mechanisms) |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
I don't think it's bloated, and I don't personally think that it should be split into separate projects. The purpose of the suite is to have "blackbox-style" tests for compiler as component. It includes testing interop, signatures generation, refassembles, and all other compiler features. It may need some restructuring when other tests are moved here, but not separation, as it will, in my opinion, lead to even more confusion on where certain new test should reside. |
This is a movement of a suite testing a full signature roundtrip:
When doing this, two new issues regarding signature generation were opened.
The tests used to work by invoking fsc.exe, is now done in memory using component tests.
Also, the verification in the older suite most likely did not catch compiler issues, because I had to fight many.
This is now captured in the test assert, that compilation must succeed.