frontend: use vite's mechanism for env variables to fill window.envir… #3268

BacLuc · 2023-02-18T15:09:40Z

…onment

That we don't have locally cached environment.js files breaking the development setup. Sadly we still use vue-cli to run our tests, so there the vite mechanism does not work and we have to maintain one additional environment.js.

In production, we compose the environment.js anyway in the configmap, thus we don't have to consider the production case in dev-environment.js.

Now we can also remove the check in index.html.
In a production environment, you have to check yourself that you have all the needed configurations set.

closes #3267

EDIT:
~~Current state: could not get the tests to run... -> converted it to draft again~~

EDIT:
Now turned the PR around:

Step 1:
replace jest with vitest
See the single commits for some quirky fixes that it works.
Also reduces the coverage by 6% in the frontend, and the overall coverage by 30%

Step 2:
Use the vite env variable mechanism to fill environmnet

I will use review comments in the files that we can discuss the different fixes that were needed.

github-actions · 2023-02-18T15:32:21Z

⛔ Feature branch deployment currently inactive.

If the PR is still open, you can add the deploy! label to this PR to trigger a feature branch deployment.

usu

We also have a environment.js file in the .jest folder: https://github.com/ecamp/ecamp3/blob/devel/frontend/.jest/environment.js

This is still needed and properly used, right?

frontend/src/dev-environment.js

frontend/src/main.js

BacLuc · 2023-02-19T11:59:43Z

We also have a environment.js file in the .jest folder: https://github.com/ecamp/ecamp3/blob/devel/frontend/.jest/environment.js

This is still needed and properly used, right?

Sadly we still use vue-cli to run our tests, so there the vite mechanism does not work and we have to maintain one additional environment.js.

carlobeltrame · 2023-02-19T22:42:57Z

Migrating from jest / vue-cli to vitest shouldn't be too hard. Maybe I'll try that soon

frontend/src/App.vue

BacLuc · 2023-02-20T20:01:30Z

Migrating from jest / vue-cli to vitest shouldn't be too hard. Maybe I'll try that soon

I tried it. They kept the api from jest, but there is not e a drop in replacement for everything. https://github.com/BacLuc/ecamp3/tree/try-vitest

carlobeltrame · 2023-02-27T16:47:47Z

I've gotten a lot further in the vitest migration, and am still actively working on this. Just FYI, so we don't do too much parallel work.
As far as I can tell so far, vite does have drop-in replacements for most if not all jest mechanisms which we use. Admittedly, some of them aren't documented very clearly in the docs. In other places, you threw out our much needed jest extensions / customizations without replacing them (e.g. the snapshot serializer works out of the box in vitest, and for the canvas mock there is an adapter package). And in yet some other places, we had to wait for some vitest bugfixes.

BacLuc · 2023-08-06T11:07:10Z

Should i

continue this PR [👍]
Help @carlobeltrame with migrating to vitest (i just need a link to your branch) [🚀]
close this PR [👎]

carlobeltrame · 2023-08-06T11:42:30Z

My work on top of yours is now here: https://github.com/carlobeltrame/ecamp3/tree/try-vitest
Right now I don't remember how far I got or what was the immediate next problem. But a good start would probably be to rebase the branch and then update all touched npm packages to their latest versions. As mentioned, some of the involved packages also had some bugfixing to do.

BacLuc · 2023-08-07T10:16:45Z

common/eslint-local-rules/__tests__/matchingTranslationKeys.spec.js

@@ -39,134 +41,133 @@ ruleTester.run('local-rules/matching-translation-keys', ruleInstance, {
      name: 'allows correct key in js',
      code: '$tc("components.hello.world")',
      options: options,
-      filename: '/app/src/components/hello.js',
+      filename: '/src/components/hello.js',


I did not get the packageDirectory mock to work, so i used a simple solution.
Else it works in the docker container, but not when run locally with an interpreter.
ok @carlobeltrame ?

As long as the tests work in CI and when run in the way we have it documented in the wiki, fine by me

BacLuc · 2023-08-07T10:18:13Z

frontend/package.json

-    "collectCoverageFrom": [
-      "src/**/*.{js,vue}",
-      "common/**/*.js",
-      "!src/components/print/print-react/generatePdf.js"
-    ],


Maybe we need to use this config also for the v8 coverage.
Should i do this in this PR, or can we do this in another PR?

I would merge this and then see whether we have any coverage problems, and solve those later.

BacLuc · 2023-08-07T10:19:53Z

frontend/src/components/form/api/__tests__/ApiRichtext.spec.js

I did not get these tests to run.

commit message:
frontend: delete RichText tests

Because i could not get them to run with vitest.
The error i could not resolve:
FAIL src/components/form/api/tests/ApiRichtext.spec.js > An ApiRichtext > updates state if value in store is refreshed and has new value
TypeError: tippy is not a function
❯ BubbleMenuView.createTooltip node_modules/@tiptap/extension-bubble-menu/src/bubble-menu-plugin.ts:141:18
❯ BubbleMenuView.updateHandler node_modules/@tiptap/extension-bubble-menu/src/bubble-menu-plugin.ts:200:10
❯ BubbleMenuView.update node_modules/@tiptap/extension-bubble-menu/src/bubble-menu-plugin.ts:170:10
❯ EditorView.updatePluginViews node_modules/prosemirror-view/dist/index.js:5262:32
❯ EditorView.updateStateInner node_modules/prosemirror-view/dist/index.js:5211:14
❯ EditorView.updateState node_modules/prosemirror-view/dist/index.js:5142:14
❯ Editor.dispatchTransaction node_modules/@tiptap/core/src/Editor.ts:344:15
❯ EditorView.dispatch node_modules/prosemirror-view/dist/index.js:5471:33
❯ Object.method [as setContent] node_modules/@tiptap/core/src/CommandManager.ts:42:18
❯ VueComponent.value src/components/form/tiptap/TiptapEditor.vue:174:1
172| // of the input field
173| if (val !== this.html) {
174| this.editor.commands.setContent(val)
| ^
175| }
176| },

Alright, let's write new ones later.

BacLuc · 2023-08-07T10:21:25Z

frontend/src/components/form/api/__tests__/ApiWrapper.spec.js

@@ -1,63 +1,45 @@
-// Libraries
+import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'


Here i had problems with the virtualTIme.
It did not work as expected, so i used additional promises and resolvers to control the execution of promises.

BacLuc · 2023-08-07T10:23:18Z

frontend/src/plugins/store/index.js

@@ -41,4 +42,6 @@ class StorePlugin {
 export let apiStore
 export let store

+export let storePlugin = new StorePlugin()


this is for the auth.spec.js test.
As soon as i started mocking the router, store.replaceState was undefined...?

I think I was able to simplify this: b33f67e

BacLuc · 2023-08-07T10:24:10Z

frontend/src/views/camp/__tests__/Admin.spec.js

+  }
+}
+
+function createStubs() {


here i had to change the stups to CamelCase

carlobeltrame · 2023-09-12T19:29:53Z

common/eslint-local-rules/__tests__/matchingTranslationKeys.spec.js

@@ -39,134 +41,133 @@ ruleTester.run('local-rules/matching-translation-keys', ruleInstance, {
      name: 'allows correct key in js',
      code: '$tc("components.hello.world")',
      options: options,
-      filename: '/app/src/components/hello.js',
+      filename: '/src/components/hello.js',


As long as the tests work in CI and when run in the way we have it documented in the wiki, fine by me

carlobeltrame · 2023-09-12T19:32:02Z

frontend/package.json

-    "collectCoverageFrom": [
-      "src/**/*.{js,vue}",
-      "common/**/*.js",
-      "!src/components/print/print-react/generatePdf.js"
-    ],


I would merge this and then see whether we have any coverage problems, and solve those later.

carlobeltrame · 2023-09-12T19:33:09Z

frontend/src/components/form/api/__tests__/ApiRichtext.spec.js

Alright, let's write new ones later.

carlobeltrame · 2023-09-12T19:36:11Z

frontend/src/components/form/api/__tests__/ApiWrapper.spec.js

+    set: lodash.set,
+    get: lodash.get,


That shouldn't be necessary after the ...lodash above..?

i also expected that, but it did not work without setting these to explicitly.
Did you test it and it worked?

carlobeltrame · 2023-09-12T19:40:02Z

frontend/src/components/form/api/__tests__/ApiWrapper.spec.js

+  const lodash = await importOriginal()
+  return {
+    ...lodash,
+    debounce: (callback) => new Promise((resolve) => (debounce = resolve)).then(callback),


Looks very funky... This implementation only supports one debounce at a time, because there is only one global let debounce, right? But I guess it is fine for now.

yes, but why would you have 2 debounce before a user interaction/web request/response...?

It is possible to write a test where e.g. the user triggers two separate debounced buttons, and check whether everything is executed in the expected order. I think this kind of setup is not supported by this mock.

carlobeltrame · 2023-09-12T19:49:02Z

frontend/src/plugins/store/index.js

@@ -41,4 +42,6 @@ class StorePlugin {
 export let apiStore
 export let store

+export let storePlugin = new StorePlugin()


I think I was able to simplify this: b33f67e

manuelmeister

Worked on my machine ;)

Not all tests are running

Because vitest cannot resolve the files if there is no extension. And update snapshots.

Vitest said correctly that this method does not exist.

Let the test helpers create the store, just provide a json. This fixes Error: [vuex] getters should be function but "getters.getLoggedInUser" is {"_meta":{"self":"/users/17d341a80579"}}. Write the stubs in camel case And simplify a little.

Because i could not get them to run with vitest. The error i could not resolve: FAIL src/components/form/api/__tests__/ApiRichtext.spec.js > An ApiRichtext > updates state if value in store is refreshed and has new value TypeError: tippy is not a function ❯ BubbleMenuView.createTooltip node_modules/@tiptap/extension-bubble-menu/src/bubble-menu-plugin.ts:141:18 ❯ BubbleMenuView.updateHandler node_modules/@tiptap/extension-bubble-menu/src/bubble-menu-plugin.ts:200:10 ❯ BubbleMenuView.update node_modules/@tiptap/extension-bubble-menu/src/bubble-menu-plugin.ts:170:10 ❯ EditorView.updatePluginViews node_modules/prosemirror-view/dist/index.js:5262:32 ❯ EditorView.updateStateInner node_modules/prosemirror-view/dist/index.js:5211:14 ❯ EditorView.updateState node_modules/prosemirror-view/dist/index.js:5142:14 ❯ Editor.dispatchTransaction node_modules/@tiptap/core/src/Editor.ts:344:15 ❯ EditorView.dispatch node_modules/prosemirror-view/dist/index.js:5471:33 ❯ Object.method [as setContent] node_modules/@tiptap/core/src/CommandManager.ts:42:18 ❯ VueComponent.value src/components/form/tiptap/TiptapEditor.vue:174:1 172| // of the input field 173| if (val !== this.html) { 174| this.editor.commands.setContent(val) | ^ 175| } 176| },

router.push takes ca 2 seconds, which is too long for a unit tests. Thus we mock router.push and router.resolve. Then store.replace does not work anymore, thus we use vi.importActual to make sure not the somehow mocked store is used.

…t for vitest Mocking seems a little difficult with vitest. If we keep the /app, the tests work in the docker container, but not when running the tests with a local interpreter.

…onment That we don't have locally cached environment.js files breaking the development setup. Sadly we still use vue-cli to run our tests, so there the vite mechanism does not work and we have to maintain one additional environment.js. In production, we compose the environment.js anyway in the configmap, thus we don't have to consider the production case in dev-environment.js. Now we can also remove the check in index.html. In a production environment, you have to check yourself that you have all the needed configurations set. closes ecamp#3267

BacLuc added the deploy! Creates a feature branch deployment for this PR label Feb 18, 2023

BacLuc force-pushed the dev-setup-improve-frontend-env-experiance branch from fac673c to 7a7d916 Compare February 18, 2023 15:10

usu reviewed Feb 19, 2023

View reviewed changes

frontend/src/dev-environment.js Outdated Show resolved Hide resolved

frontend/src/main.js Outdated Show resolved Hide resolved

BacLuc removed the deploy! Creates a feature branch deployment for this PR label Feb 20, 2023

BacLuc force-pushed the dev-setup-improve-frontend-env-experiance branch 3 times, most recently from 7397544 to b9b475d Compare February 20, 2023 17:40

BacLuc added the deploy! Creates a feature branch deployment for this PR label Feb 20, 2023

BacLuc commented Feb 20, 2023

View reviewed changes

frontend/src/App.vue Outdated Show resolved Hide resolved

BacLuc marked this pull request as draft February 20, 2023 20:00

BacLuc removed the deploy! Creates a feature branch deployment for this PR label Feb 20, 2023

carlobeltrame self-assigned this Feb 27, 2023

BacLuc mentioned this pull request Jul 15, 2023

Fix warnings in frontend tests #3625

Closed

BacLuc force-pushed the dev-setup-improve-frontend-env-experiance branch 5 times, most recently from af943ea to db0e26d Compare August 7, 2023 09:57

BacLuc commented Aug 7, 2023

View reviewed changes

BacLuc marked this pull request as ready for review August 7, 2023 10:24

BacLuc requested review from usu and carlobeltrame August 7, 2023 10:25

BacLuc mentioned this pull request Aug 15, 2023

PrintConfigurator.vue: fix error when collecting coverage #3516

Closed

carlobeltrame approved these changes Sep 12, 2023

View reviewed changes

BacLuc requested review from manuelmeister and simfeld September 15, 2023 20:26

manuelmeister approved these changes Sep 16, 2023

View reviewed changes

BacLuc added 11 commits September 17, 2023 11:17

frontend: minimal switch to vitest

9c5ff93

Not all tests are running

frontend: add .vue to imports for tests

1ba2cbd

Because vitest cannot resolve the files if there is no extension. And update snapshots.

frontend: move method debouncedSave to methods where it belongs

4e408be

Vitest said correctly that this method does not exist.

frontend: fix ApiWrapper.spec.js for vitests

24dc4bf

ApiMock.js: replace jest with vi

60c9a75

Admin.spec.js: adapt to vitest

f6ebc07

Let the test helpers create the store, just provide a json. This fixes Error: [vuex] getters should be function but "getters.getLoggedInUser" is {"_meta":{"self":"/users/17d341a80579"}}. Write the stubs in camel case And simplify a little.

auth.spec.js: reduce time for logout test

e00cf78

router.push takes ca 2 seconds, which is too long for a unit tests. Thus we mock router.push and router.resolve. Then store.replace does not work anymore, thus we use vi.importActual to make sure not the somehow mocked store is used.

frontend: configure coverage for vitest

9a63a67

common: make matchingTranslationKeys.spec.js run directory independen…

a7cc8fd

…t for vitest Mocking seems a little difficult with vitest. If we keep the /app, the tests work in the docker container, but not when running the tests with a local interpreter.

BacLuc force-pushed the dev-setup-improve-frontend-env-experiance branch from c9193ee to bb570c7 Compare September 17, 2023 09:38

BacLuc added the deploy! Creates a feature branch deployment for this PR label Sep 17, 2023

BacLuc temporarily deployed to feature-branch September 17, 2023 09:45 — with GitHub Actions Inactive

BacLuc enabled auto-merge September 17, 2023 09:55

BacLuc added this pull request to the merge queue Sep 17, 2023

Merged via the queue into ecamp:devel with commit 225105f Sep 17, 2023
25 checks passed

BacLuc deleted the dev-setup-improve-frontend-env-experiance branch September 17, 2023 10:27

This was referenced Sep 19, 2023

Frontend: fix vitest warnings #3812

Merged

Improve saving indicator #3609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frontend: use vite's mechanism for env variables to fill window.envir… #3268

frontend: use vite's mechanism for env variables to fill window.envir… #3268

BacLuc commented Feb 18, 2023 •

edited

Loading

github-actions bot commented Feb 18, 2023 •

edited

Loading

usu left a comment

BacLuc commented Feb 19, 2023

carlobeltrame commented Feb 19, 2023

BacLuc commented Feb 20, 2023 •

edited

Loading

carlobeltrame commented Feb 27, 2023

BacLuc commented Aug 6, 2023 •

edited by manuelmeister

Loading

carlobeltrame commented Aug 6, 2023

BacLuc Aug 7, 2023

carlobeltrame Sep 12, 2023

BacLuc Aug 7, 2023

carlobeltrame Sep 12, 2023

BacLuc Aug 7, 2023

carlobeltrame Sep 12, 2023

BacLuc Aug 7, 2023

BacLuc Aug 7, 2023

carlobeltrame Sep 12, 2023

BacLuc Aug 7, 2023

carlobeltrame Sep 12, 2023

carlobeltrame Sep 12, 2023

carlobeltrame Sep 12, 2023

carlobeltrame Sep 12, 2023

BacLuc Sep 12, 2023

carlobeltrame Sep 12, 2023

BacLuc Sep 12, 2023

carlobeltrame Sep 13, 2023

carlobeltrame Sep 12, 2023

manuelmeister left a comment

		@@ -1,63 +1,45 @@
		// Libraries
		import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'

frontend: use vite's mechanism for env variables to fill window.envir… #3268

frontend: use vite's mechanism for env variables to fill window.envir… #3268

Conversation

BacLuc commented Feb 18, 2023 • edited Loading

github-actions bot commented Feb 18, 2023 • edited Loading

⛔ Feature branch deployment currently inactive.

usu left a comment

Choose a reason for hiding this comment

BacLuc commented Feb 19, 2023

carlobeltrame commented Feb 19, 2023

BacLuc commented Feb 20, 2023 • edited Loading

carlobeltrame commented Feb 27, 2023

BacLuc commented Aug 6, 2023 • edited by manuelmeister Loading

carlobeltrame commented Aug 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuelmeister left a comment

Choose a reason for hiding this comment

BacLuc commented Feb 18, 2023 •

edited

Loading

github-actions bot commented Feb 18, 2023 •

edited

Loading

BacLuc commented Feb 20, 2023 •

edited

Loading

BacLuc commented Aug 6, 2023 •

edited by manuelmeister

Loading