SAFE-Stack / SAFE-template

dotnet CLI template for SAFE project
MIT License
280 stars 87 forks source link

Fix bug issue #615 in Client.Tests.fs #616

Closed giuliohome closed 2 weeks ago

giuliohome commented 3 weeks ago

Fix bug issue #615

Helping Copilot to explain the fix, ex-post.

giuliohome: explain why adding line "let model, _ = update (LoadTodos (Finished [])) model" fix a bug so that the test does not fail now

GitHub Copilot: The line let model, _ = update (LoadTodos (Finished [])) model is loading an empty list of todos into the model. This is likely fixing a bug where the model's todo list was not initialized properly, causing the test to fail.

In the previous state, the model might have been in an uninitialized state or contained some stale data. By explicitly loading an empty list of todos, it ensures that the model is in a known and clean state before the test starts to add a new todo. This makes the test more reliable and predictable, as it's not affected by the previous state of the model.

This is a common practice in unit testing, where each test should be isolated and not depend on the state from other tests. It's also part of the Arrange-Act-Assert pattern, where you first arrange (set up) the system under test, then act on it, and finally assert that it behaved as expected. In this case, loading the empty list of todos is part of the arrange phase.

Now in my words.

The issue arises because the init function generates a model with todos that have not started yet. As a result, the remote map used for the Length assertion doesn't map to any function, causing the test to fail.

giuliohome commented 2 weeks ago

Note that this PR does not address the separate question asked here: https://github.com/SAFE-Stack/SAFE-template/issues/615#issuecomment-2169434320

To answer that separate question as well, follow these steps (based on this Fable.Mocha paragraph):

  1. Runnpm install --save-dev mocha.
  2. Add a package.json file under Client/tests with the following content:
{
    "type": "module",
    "scripts": {
        "pretest": "dotnet fable Client.Tests.fsproj -o dist/tests",
        "test": "mocha dist/tests"
    },
    "devDependencies": {
        "mocha": "^9.2.2"
    }
}
  1. Run npm test

  2. Finally modify the Build.fs as follows

    Target.create "RunTests" (fun _ ->
    run dotnet [ "build" ] sharedTestsPath
    
    [
        "server", dotnet [ "watch"; "run" ] serverTestsPath
        "mocha", npm [ "test" ] clientTestsPath
        "client", dotnet [ "fable"; "watch"; "-o"; "output"; "-s"; "--run"; "npx"; "vite" ] clientTestsPath
    ]
    |> runParallel)

    immagine

giuliohome commented 2 weeks ago

About Copilot and GPT

By copying the full context of the code and test files and providing competent, human-given directions, GPT-4o (rather than Copilot) is eventually able to find the fix. See https://chatgpt.com/share/45bf6f26-85a3-463f-9a57-ce4476afc82a

immagine

immagine

Larocceau commented 2 weeks ago

@giuliohome thanks for your contribution, it has been merged and released. We'd like to keep the discussion #615 going to determine whether we'd like this to be the final solution, or make the demo app slightly more resilient by making sure the "add todo" operation can work even if loading the todos somehow failed.