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

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

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

giuliohome
Copy link
Contributor

@giuliohome giuliohome commented Jun 16, 2024

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
Copy link
Contributor Author

giuliohome commented Jun 17, 2024

Note that this PR does not address the separate question asked here: #615 (comment)

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

  1. Run npm 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
Copy link
Contributor Author

giuliohome commented Jun 17, 2024

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 Larocceau self-requested a review June 21, 2024 16:15
@Larocceau Larocceau merged commit 2fdf03f into SAFE-Stack:master Jun 21, 2024
1 check passed
@Larocceau
Copy link
Contributor

Larocceau commented Jun 21, 2024

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

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.

2 participants