Skip to content

Simplified compiler test framework #9721

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.

Sign up for GitHub

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

Jump to bottom
Merged
vzarytovskii merged 21 commits into from
Jul 23, 2020
Merged

Simplified compiler test framework #9721

vzarytovskii merged 21 commits into from
Jul 23, 2020

Conversation

@vzarytovskii
Copy link
Member

@vzarytovskii vzarytovskii commented Jul 20, 2020 •
edited
Loading

First round of implementation of #9720

There are still a bunch of stuff to implement, but first, we need to have a new base for it.

@vzarytovskii vzarytovskii changed the title [WIP] Simplified compiler test framework [WIP] [Don't review] Simplified compiler test framework Jul 20, 2020
@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jul 20, 2020 •
edited
Loading

Not really a review here, just a suggestion:

May I suggest we improve withWarning to something like withWarning (Warning 213, (Line 2, Col 1, Line 2, Col 6)), or similarly, withWarning (Warning 213, Pos.From(2, 1), Pos.To(2, 6))? I find these notoriously hard to read when working with tests (though everything here is already a biiig improvement!).

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Jul 20, 2020

Not really a review here, just a suggestion:

May I suggest we improve withWarning to something like withWarning (Warning 213, (Line 2, Col 1, Line 2, Col 6)), or similarly, withWarning (Warning 213, Pos.From(2, 1), Pos.To(2, 6))? I find these notoriously hard to read when working with tests (though everything here is already a biiig improvement!).

Yeah, that's the plan - I will add a verbose versions (looks pretty much like you suggested), as well as fewer small functions which are easier to compose (like withErrorCode 123 |> onPosition ... |> withMessage ... etc), so you have freedom to how to write and how vebose you want it :)

Also - I would love to chat about the more complex testing scenarios (like one you've encountered in ExprTests).

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jul 20, 2020 •
edited
Loading

Also - I would love to chat about the more complex testing scenarios (like one you've encountered in ExprTests).

Sure, any time, you can ping me on Slack (just remember I'm on European time ;) ) and we can set up a call.

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Jul 20, 2020

Also - I would love to chat about the more complex testing scenarios (like one you've encountered in ExprTests).

Sure, any time, you can ping me on Slack (just remember I'm on European time ;) ) and we can set up a call.

Yeah, I'm in Prague myself :)
Will ping you there some time during the week then.

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jul 20, 2020 •
edited
Loading

Oh I love Prague! I come there at least once a year to visit friends and for a conf. I know it quite well, and of course the 🍺 🍻 🍺

@vzarytovskii vzarytovskii changed the title [WIP] [Don't review] Simplified compiler test framework Simplified compiler test framework Jul 22, 2020
@vzarytovskii vzarytovskii marked this pull request as ready for review July 22, 2020 15:36
cartermp
cartermp reviewed Jul 22, 2020
View reviewed changes
tests/FSharp.Test.Utilities/CompilerAssert.fs

// NOTE: This function will not clean up all the compiled projects after itself.
// The reason behind is so we can compose verification of test runs easier.
// TODO: We must not rely on the filesystem when compiling
Copy link
Contributor

@cartermp cartermp Jul 22, 2020

Choose a reason for hiding this comment

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

How will that be resolved?

Copy link
Member Author

@vzarytovskii vzarytovskii Jul 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I have a separate issue for tackling that.
I'm thinking on the suite teardown level we can clean up all projects/sources we've created.

KevinRansom
KevinRansom approved these changes Jul 23, 2020
View reviewed changes
Copy link
Contributor

@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.

Looks, great. I have suggested a few things to consider in the next update. But they shouldn't hold up merging this PR.

Nice work

Kevin

tests/FSharp.Test.Utilities/Compiler.fs
Warnings = []
Errors = [] }

let private typecheckFSharpSource (fsSource: FSharpCompilationSource) : CompilationResult =
Copy link
Contributor

@KevinRansom KevinRansom Jul 23, 2020

Choose a reason for hiding this comment

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

Is it possible we would like have tests with multiple source files?

Copy link
Member Author

@vzarytovskii vzarytovskii Jul 23, 2020

Choose a reason for hiding this comment

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

Yep, it's tracked in #9746

@KevinRansom KevinRansom reopened this Jul 23, 2020
@vzarytovskii vzarytovskii merged commit 3f011ec into dotnet:master Jul 23, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
@vzarytovskii @cartermp @nosami
Simplified compiler test framework (dotnet#9721)
ab31d12 
* [WIP] Simplified compiler API for testing

* [WIP] Support of references, for F#, C# projects. Added more asserts.

* Added more examples + ignoreWarnings support

* Added typecheck + baseline test helpers.

* Added running functionality

* Moved a bunch of compiler tests to a new suite

* Changed error message

* [WIP] Added CSharp compilation + referencing

* Added some simple interop tests

* [WIP] Process MetadataReference recursively

* [WIP] Added netcorapp31 support for tests

* [WIP] Termporary revert to using netcoreapp30 in the tests (instead of netcoreapp31)

* Added types to errors/warnings as oppose to just having tuples

* Fixed tests to fit newer API

* Revert libraries upgrade

* Revert change netcoreapp31 -> netcoreapp30

* Update tests/FSharp.Test.Utilities/Compiler.fs

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* Addressed some PR comments

* Fixed missing styling inconsistensies

* Addressed feedback: recursive module instead of forward type declarations + fixed options composition.

* Changed Option.isSome -> defaultArg

Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@jonsequitur jonsequitur jonsequitur left review comments

@cartermp cartermp cartermp left review comments

@KevinRansom KevinRansom KevinRansom approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vzarytovskii @abelbraaksma @jonsequitur @KevinRansom @cartermp