SAFE-Stack / SAFE-template

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

Failing Test #615

Open giuliohome opened 2 weeks ago

giuliohome commented 2 weeks ago

Fable, you have a template with a failing test—why?!

dotnet new install SAFE.Template
dotnet new SAFE
dotnet tool restore
dotnet run -- RunTests

immagine

https://github.com/SAFE-Stack/SAFE-template/blob/e8677f329079b5741f587289526b9fe18f00675b/Content/default/tests/Client/Client.Tests.fs#L16)

Is one supposed to fix it this way?

let model, _ = update (LoadTodos (Finished [newTodo])) model

or if you prefer

let model, _ = update (LoadTodos (Finished [])) model
let model, _ = update (SaveTodo(Finished newTodo)) model

Is it a template or an examination/test?

Comment about using Copilot

Copilot and GPT cannot pass it.

immagine

I'm happy that Copilot cannot do that because it means human programmers' jobs are saved! 😄

immagine

But Copilot actually helps because after the above changes you will see that the root cause error is "Todos were not in the Loaded state".

immagine

But Copilot actually helps because, after making the above changes, you will see that the root cause of the error is 'Todos were not in the Loaded state,' even though it is not really capable of producing the correct fix for the problem.

giuliohome commented 2 weeks ago

Separate question.

Besides fixing the test (as above), one must first understand that server project tests appear in the console output, whereas client project tests appear in the browser. This can be quite confusing for beginners and turns it into a specialized template. For experts too, it might be convenient, if not necessary, to get the return code of the client tests (and possibly the output) from the console as well as from the browser. Would it be possible to modify the template to achieve this?

isaacabraham commented 2 weeks ago

@giuliohome If I remember correctly, the test deliberately fails to show you how a failing test will appear in the app! But I'll double check.

Larocceau commented 1 week ago

I had a look at v. 4.3.0. the test does pass there, which makes me believe this test failing is not intentional. If we really want a test that fails deliberately I would say that should be a very obvious like Expect.equal true false.

I feel the test failing does actually show a bit of funk that results from using the RemoteData type, and using RemoteData.map function to update data: If the assumption that the remote data is successfully loaded is violated, you'll be able to add new todo's, but not see them show up in the list.

I'd say the most complete fix would be to change the addTodo api endpoint to Todo -> Async<Todo list>, always returning the full todo list, so we don't rely on previously received state on the front end to process added todo nicely in the front end.

@isaacabraham, thoughts?