Consolidate interactive tests #482

KevinRansom · 2019-09-26T03:34:08Z

First attempt at rationalizing F# and C# interactive test cases.

Simplify and refactor: CSharpKernelTests/FSharpKernelTests/CSharpKernelTestBase/FSharpKernelTestBase/KernelTestBase into

LanguageKernelTests/LanguageKernelTestBase

Here is an example test case with C# and F# source variants:
https://github.com/dotnet/try/compare/master...KevinRansom:kr?expand=1#diff-d8de318123580f112042b15d5a087f4dR55

I really wish this was in F# the amount of ceremony in C# is unbelievable
In fact I may port this whole thing to F# just to show you.

        [Theory]
        [InlineData(Language.FSharp)]
        [InlineData(Language.CSharp)]
        public async Task it_remembers_state_between_submissions(Language language)
        {
            string[] source = null;
            switch (language)
            {
                case Language.FSharp:
                    string[] fsLines =
                    {
                        "let add x y = x + y",
                        "add 2 3"
                    };
                    source = fsLines;
                    break;
                case Language.CSharp:
                    string[] csLines =
                    {
                        "int Multiply(int x, int y) { return x + y; };",
                        "Multiply(2, 3);"
                    };
                    source = csLines;
                    break;
            }

            var kernel = CreateKernel(language);

            await SubmitLines(kernel, source);

            KernelEvents.ValuesOnly()
                .OfType<ReturnValueProduced>()
                .Last()
                .Value
                .Should()
                .Be(5);
        }

jonsequitur · 2019-09-26T15:56:12Z

WorkspaceServer.Tests/Kernel/LanguageKernelTestBase.cs

+        }
+
+        public async Task SubmitLines(KernelBase kernel, string[] lines)
+        {


The naming is a bit misleading since multi-line submissions are very common.

Something like string[] codeSubmissions or codeFragments?

I think submissions is fine.

jonsequitur · 2019-09-26T16:03:24Z

Generally, agreed, though a switch expression would make this example a little more concise:

        [Theory]
        [InlineData(Language.FSharp)]
        [InlineData(Language.CSharp)]
        public async Task it_remembers_state_between_submissions(Language language)
        {
            var source = language switch
            {
                Language.FSharp => new[]
                {
                    "let add x y = x + y",
                    "add 2 3"
                },
                Language.CSharp => new[]
                {
                    "int Multiply(int x, int y) { return x + y; };",
                    "Multiply(2, 3);"
                }
            };

            var kernel = CreateKernel(language);

            await SubmitLines(kernel, source);

            KernelEvents.ValuesOnly()
                        .OfType<ReturnValueProduced>()
                        .Last()
                        .Value
                        .Should()
                        .Be(5);
        }

jonsequitur · 2019-09-26T16:04:24Z

It would be helpful for reviewers to spot possible errors if you do the renames in a prior commit to the actual changes.

brettfo · 2019-09-26T16:35:38Z

WorkspaceServer.Tests/Kernel/LanguageKernelTestBase.cs

+            return kernel;
+        }
+
+        protected KernelBase CreateKernel()


I'd feel better if this function was removed. Tests that call this are really C#-specific tests so I'd like them to explicitly call CreateKernel(CSharp) or group them in a new class with IKernel CreateKernel() => base.CreateKernel(CSharp); just so it's explicit..

brettfo · 2019-09-26T16:36:19Z

WorkspaceServer.Tests/Kernel/LanguageKernelTestBase.cs

+            return CreateKernel(Language.CSharp);
+        }
+
+        public async Task SubmitLines(KernelBase kernel, string[] lines)


params string[] lines might be easier to consume, but ultimately I don't care too much.

It propose using submission or even array of string array

brettfo · 2019-09-26T16:42:37Z

General request: To make the test cases easier to parse, could the switch statements/expressions be replaced with this?

// I'm not married to the name of this function, but I think the use of `T` will make it easy to in the future return more than just strings.
public T GetElementForLanguage<T>(Language language, params (Language lang, T value)[] values)
{
    // The use of `Single()` ensures that the test will fail if a value was not given for a specific language.
    return values.Single(val => val.lang == language).value;
}

That way the call sites will be a little more concise:

var code = GetElementForLanguage(language, (Language.CSharp, "using System;"),
                                           (Language.FSharp, "open System"));

KevinRansom · 2019-09-26T17:57:01Z

Generally, agreed, though a switch expression would make this example a little more concise:

@jonsequitur, I tried it, unfortunately an array initializer is not an expression that can be returned from a switch expression.

KevinRansom · 2019-09-26T18:03:46Z

Generally, agreed, though a switch expression would make this example a little more concise:

@jonsequitur, I tried it, unfortunately an array initializer is not an expression that can be returned from a switch expression.

Oh the trick is to use new[]. Nice.

KevinRansom · 2019-09-26T19:35:51Z

I have added 4 different ways of doing a test based on the feedback, and other ideas. let me know what you like best.

brettfo · 2019-09-26T19:50:30Z

WorkspaceServer.Tests/Kernel/LanguageKernelTests.cs

+        [Theory]
+        [InlineData(Language.FSharp)]
+        [InlineData(Language.CSharp)]
+        public async Task it_remembers_state_between_submissions_option3(Language language)


I'm going to cast my vote for this style of test case because:

The code to select different values is short and easy to read. I find the switch expression difficult because it's so verbose and requires a default case that must explicitly throw, while the helper method here handles that for you.

Placing code in [InlineData] works for compile-time constants, but won't be usable for expressions. We don't have a test that does this currently, but I can easily see a test that needs to call GetElementForLanguage() where the item returned is a Type or Exception or something that can't be placed in an attribute.

Finally, I like that the code variants are declared exactly where and when they're used instead of at the top of the function where I have to trace a variable through.

colombod · 2019-09-26T16:39:09Z

WorkspaceServer.Tests/Kernel/LanguageKernelTests.cs

+            string[] source = null;
+            switch (language)
+            {
+                case Language.FSharp:


I think it would be easier to provide a list of submissions instead of assuming one stein one submission.

colombod · 2019-09-26T16:40:29Z

WorkspaceServer.Tests/Kernel/LanguageKernelTestBase.cs

+            return CreateKernel(Language.CSharp);
+        }
+
+        public async Task SubmitLines(KernelBase kernel, string[] lines)


It propose using submission or even array of string array

colombod · 2019-09-26T16:41:36Z

WorkspaceServer.Tests/Kernel/LanguageKernelTestBase.cs

+        }
+
+        public async Task SubmitLines(KernelBase kernel, string[] lines)
+        {


Something like string[] codeSubmissions or codeFragments?

WorkspaceServer.Tests/Kernel/LanguageKernelTestBase.cs

KevinRansom · 2019-10-01T05:45:11Z

@colombod,

SubmitSource is an overloaded method, so you can pass in a line or an array of lines. If we determine that we need an array of arrays then we can always add that overload when the need presents itself.

Kevin

Merge

jonsequitur · 2019-10-02T16:19:22Z

WorkspaceServer.Tests/Kernel/LanguageKernelTests.cs

+
+            KernelEvents
+                .ValuesOnly()
+                .Single(e => e is IncompleteCodeSubmissionReceived);


.Should().ContainSingle()

jonsequitur · 2019-10-02T16:20:34Z

WorkspaceServer.Tests/Kernel/LanguageKernelTests.cs

+            var source = language switch
+            {
+                Language.FSharp => "Console.WriteLine(\"Hello\")",
+                Language.CSharp => "Console.WriteLine(\"Hello\")"


In this case the test will pass for the wrong reason because this is not a complete C# statement unless you add a semicolon to the end.

@jonsequitur
Console.WriteLine("Hello, World") is a complete C# expression, if it was incomplete this would fail:

KernelEvents .Should() .Contain(e => e.Value is CompleteCodeSubmissionReceived);

This is the notebook with a C# kernel note no ';'

jonsequitur · 2019-10-02T16:23:18Z

WorkspaceServer.Tests/Kernel/LanguageKernelTests.cs

+json
+"},
+                Language.CSharp => new[] {
+$"#r @\"{dllPath}\"",


$"#r @"{dllPath}"" [](start = 0, length = 20)

I believe this works if you remove the @. The issue is not the quoted path but rather than the command line parser expects this to be a response file. That can be disabled in KernelBase.GetDirectiveParser:

= new CommandLineBuilder(root) .ParseResponseFileAs(ResponseFileHandling.Disabled) .UseMiddleware(

Ahh … nice.

Consolidate interactive tests

35243cb

KevinRansom requested review from TIHan, brettfo and jonsequitur September 26, 2019 03:34

jonsequitur requested a review from colombod September 26, 2019 15:53

jonsequitur reviewed Sep 26, 2019

View reviewed changes

brettfo reviewed Sep 26, 2019

View reviewed changes

Show options

6a4e1c5

brettfo reviewed Sep 26, 2019

View reviewed changes

updated

187633f

colombod reviewed Sep 26, 2019

View reviewed changes

KevinRansom and others added 2 commits September 26, 2019 20:29

extend test cases

127e9aa

merge

1c5f5fa

jonsequitur reviewed Sep 27, 2019

View reviewed changes

WorkspaceServer.Tests/Kernel/LanguageKernelTestBase.cs Show resolved Hide resolved

jonsequitur reviewed Sep 27, 2019

View reviewed changes

WorkspaceServer.Tests/Kernel/LanguageKernelTestBase.cs Outdated Show resolved Hide resolved

KevinRansom and others added 5 commits September 27, 2019 16:25

Merge branch 'master' of https://github.com/dotnet/try into kr

fca3e01

Get all C# tests working again

414b4ab

More stuff

9d7109a

temp

6f1a883

Tests should now pass with todos -- for failing test cases

ccabef3

KevinRansom added 2 commits September 30, 2019 22:53

feedback

004e394

todo

c064836

KevinRansom added 2 commits October 1, 2019 13:39

Merge branch 'master' of https://github.com/dotnet/try into kr

00a06ef

Fix some test issues

6b55667

KevinRansom force-pushed the kr branch from 5aa553c to 6b55667 Compare October 1, 2019 21:55

KevinRansom and others added 2 commits October 1, 2019 14:57

Update .gitignore

f319649

Merge

Clean up

a77ee34

jonsequitur reviewed Oct 2, 2019

View reviewed changes

brettfo mentioned this pull request Oct 2, 2019

capture console output from F# kernel #495

Merged

KevinRansom and others added 7 commits October 2, 2019 16:09

Something changed

37d1394

merge

318fdd4

re-delete CSharpKernelTests.cs

11358ef

Merge branch 'master' of https://github.com/dotnet/try into kr

4b94922

Revert some tests back to DisplayedValueProduced

ade9cfd

merge

19121c7

StandardOutput tests

b7ded43

KevinRansom force-pushed the kr branch from d95dfa6 to b7ded43 Compare October 4, 2019 09:17

jonsequitur approved these changes Oct 7, 2019

View reviewed changes

colombod merged commit e43a0ab into dotnet:master Oct 7, 2019

snyk-bot mentioned this pull request Mar 30, 2022

[Snyk] Upgrade react-monaco-editor from 0.18.0 to 0.47.0 yangzhigang1999/try#5

Open

Consolidate interactive tests #482

Consolidate interactive tests #482

Uh oh!

Conversation

KevinRansom commented Sep 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonsequitur Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonsequitur commented Sep 26, 2019

Uh oh!

jonsequitur commented Sep 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettfo commented Sep 26, 2019

Uh oh!

KevinRansom commented Sep 26, 2019

Uh oh!

KevinRansom commented Sep 26, 2019

Uh oh!

KevinRansom commented Sep 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KevinRansom commented Oct 1, 2019

Uh oh!

jonsequitur Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonsequitur Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonsequitur Sep 27, 2019 •

edited

Loading

jonsequitur Oct 2, 2019 •

edited

Loading

jonsequitur Oct 2, 2019 •

edited

Loading