fix: test and visual testing update for otter training

[cpaulve-1A]

Proposed change

Related issues

- No issue associated -

[nx-cloud]

View your CI Pipeline Execution ↗ for commit bfa43cb.

Command	Status	Duration	Result
nx run-many --target=build --projects=eslint-pl...	✅ Succeeded	<1s	View ↗

---

☁️ Nx Cloud last updated this comment at 2024-12-20 13:38:00 UTC

[kpanot]

Should not use the logger instead?

[matthieu-crouzet]

I don't think we set a logger for now on the Otter showcase
If we don't have one set up, I would expect to do it in another PR

[fpaul-1A]

shouldn't we let it throw? Lot of things will be broken if we don't have the global monaco instance

[cpaulve-1A]

It will throw if the monaco editor is never instantiated and the newMonacoEditorCreated subject is completed.
This is the case in the unit tests.
I could catch the monacoPromise in the tests themselves but that would require making it public and I prefer adding a catch than transforming a private property into a public one for testing purposes.

[fpaul-1A]

we should be able to test monaco-editor in the unit-tests? only the webcontainer part should be mocked?

[kpanot]

To have a better typing for monacoPromise, maybe you can caught error in the observable and use the second parameter of firstValueFrom to define { defaultValue: undefined }

[matthieu-crouzet]

Currently the typing is Promise<void | typeof Monaco>, the void comes from the .catch so adding { defaultValue: undefined } will change the type to Promise<void | typeof Monaco | undefined>
if we want to have Promise<typeof Monaco | undefined>, we should either have

(err) => {
      console.error(err);
      return undefined;
}

or

.catch<undefined>(...)

I checked the implementation of the option defaultValue it is used only if the subject newMonacoEditorCreated complete before providing a value.
implementation of firstValueFrom

[cpaulve-1A]

Actually I think defaultValue might better fit my use case here.

[matthieu-crouzet]

Why do you have to add this ?
I don't see it on the bootstrap exemple

[cpaulve-1A]

I choose to put it here instead of inside the unit test to keep the SaveCodeDialog as a standalone, but maybe I should add it in the unit tests themselves.

[matthieu-crouzet]

ok, I don't have opinion if it should be done one UT or in the component itself

[fpaul-1A]

I had a similar problem with markdown and I had to put it on the tests otherwise the component would not receive the instance provided globally (the global config for markdown was not applied)

[cpaulve-1A]

I will go for the same solution in that case