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

Handle tests within nested modules #1844

Merged

Conversation

kojo12228
Copy link
Contributor

Fixes #1756

Using module type information from ionide/FsAutoComplete#1071, correctly handle the full name of tests where there is a module within a module, a type within a module or a module which shares the same name as a sibling type.

test_explorer_nested_modules

Unfortunately, TestAdapterEntry which holds the information about module type is not accessible at the point where the full name is constructed, and the VSCode type TestItem has no field where this information can be added (AFAIK). To workaround that, this PR uses an otherwise undesirable ResizeArray acting as a map of sorts to hold mutable state about module types and this is passed to various functions. Ideally, something like a Set would be used but I will double check if Fable supports System.Collections.Generic.HashSet (otherwise it might be worth doing some housekeeping to ensure this collection doesn't become a memory leak).

@baronfel
Copy link
Contributor

Great job! I'll wait to merge this until the matching FSAC PR is merged and released, ok?

@baronfel
Copy link
Contributor

@kojo12228 can you resolve the conflicts with the other PR when you have a moment? I'm working on an FSAC release so when that's completed I'm ready to merge and release this fix as well.

@kojo12228
Copy link
Contributor Author

@baronfel sorry for the delay, I merged them locally but was getting issues with test explorer that I don't get on main. I'll push the merge change for now

@kojo12228
Copy link
Contributor Author

Odd behaviour since merging

For further details, tests seems to be correctly being discovered with eea849e:

test_explorer_nested_modules_eea849e

However something is happening on 182b6ff that I can't explain, UnitTest1.fs and UnitTest3.fs tests don't get correctly discovered. They're using the same version of FSAC:

test_explorer_nested_modules_182b6ff

The diff between the commits (ignoring commits) is the following

Starting line 194 in this PR:

                        if Seq.length testCases > 1 then
                            let ti =
-                                t.Test.children.get (t.Test.uri.Value.ToString() + " -- " + testName)
+                                t.Test.children.get (
+                                    t.Test.uri.Value.ToString()
+                                    + " -- "
+                                    + Convert.ToBase64String(Encoding.UTF8.GetBytes(testName))
+                                )
                                |> Option.defaultWith (fun () ->
                                    tc.createTestItem (

Starting line 224 in this PR:

let rec mapTest
    (tc: TestController)
    (uri: Uri)
    (moduleTypes: Collections.Generic.Dictionary<string, string>)
    (t: TestAdapterEntry)
    : TestItem =
-    let ti = tc.createTestItem (uri.ToString() + " -- " + string t.id, t.name, uri)
+    let ti =
+        tc.createTestItem (
+            uri.ToString() + " -- " + Convert.ToBase64String(Encoding.UTF8.GetBytes(t.name)),
+            t.name,
+            uri
+        )

Starting line 271 in this PR:

-    logger.Debug("Module types", moduleTypes)

    let rec getFullName (ti: TestItem) =

@kojo12228
Copy link
Contributor Author

Also main works without issue as well, so it's something with interleaving of the two that's the issue. I find that weird, especially because I originally wrote the first commit on top of #1841

@kojo12228
Copy link
Contributor Author

kojo12228 commented Mar 12, 2023

Figured it out, #1841 unintentionally made a change such that TestItem.id was no longer unique across an entire source file (due to t.id being unique amongst the file), as it was based on the name of the method/module/namespace and nothing else. The following would silently fail:

module Outer =
    [<TestFixture>]
    module Inner =
        [<Test>]
        let Test1 () = // First Test1
            Assert.Pass ()

        module Innermost =
            [<Test>]
            let Test1 () = // Second Test1, Dictionary.Add would silently throw
                Assert.Pass()

Fixed this by passing a parentNameId string down the tree and including it in the TestItem.id.

I think we're good to go @baronfel , apologies for the delay.

@baronfel
Copy link
Contributor

No worries at all, thank you for taking the time to dig into it :) I'll try to get out a new release containing all of this later today!

@kojo12228
Copy link
Contributor Author

@baronfel works as expected with FsAutoComplete 0.59.2 (not sure if you want the commit in this branch though)

@baronfel baronfel merged commit 6c485f6 into ionide:main Mar 19, 2023
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.

Test Explorer doesn't support tests with TestCaseAttribute
2 participants