-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Adds Wasp app runner for headless tests #2534
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
Conversation
Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
|
How are we doing here, need somebody to check this out @infomiho , or is it still cooking? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Full disclosure: I forgot I was reviewing a draft so I left a bunch of comments. By the time I realized, it was to late. Anyway, you probably had most of this stuff in mind.
Hopefully there's something in there you would have missed though 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a robust script to run Wasp applications with PostgreSQL support, enabling automated database setup and migrations for headless tests.
- Introduces dependency checks and installation in dependencies.ts.
- Provides DB abstractions with specific implementations for SQLite and PostgreSQL in the db folder.
- Updates documentation and headless test configurations for example apps.
Reviewed Changes
Copilot reviewed 23 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| wasp-app-runner/src/dependencies.ts | Implements dependency check & CLI installation logic. |
| wasp-app-runner/src/db/types.ts | Defines types for running apps with databases. |
| wasp-app-runner/src/db/sqlite.ts | Implements SQLite DB runner that directly calls the app. |
| wasp-app-runner/src/db/postgres.ts | Provides PostgreSQL container handling and readiness checks. |
| wasp-app-runner/src/db/index.ts | Aggregates DB execution flows based on selected DB type. |
| wasp-app-runner/README.md | Details usage, configuration, and environment setup. |
| examples/todo-typescript/src/MainPage.tsx | Refactors button class names; changes logout logic to delete-tasks action. |
| examples/todo-typescript/headless-tests/ | Adds tests and configuration files for headless testing using Playwright. |
Files not reviewed (9)
- examples/todo-typescript/.gitignore: Language not supported
- examples/todo-typescript/package-lock.json: Language not supported
- examples/todo-typescript/package.json: Language not supported
- examples/todo-typescript/src/Main.css: Language not supported
- examples/todo-typescript/tsconfig.json: Language not supported
- wasp-app-runner/.gitignore: Language not supported
- wasp-app-runner/.npmignore: Language not supported
- wasp-app-runner/package-lock.json: Language not supported
- wasp-app-runner/package.json: Language not supported
Comments suppressed due to low confidence (2)
wasp-app-runner/src/db/postgres.ts:30
- The variable name 'doesPostgresContainerExit' appears to be a typo; consider renaming it to 'doesPostgresContainerExist' for clarity.
const doesPostgresContainerExit = await checkIfPostgresContainerExists(containerName);
examples/todo-typescript/src/MainPage.tsx:38
- The variable 'completed' is referenced but not defined; please ensure it is declared or passed as the appropriate value.
onClick={() => void deleteTasks(completed ?? [])}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fixes, but I still have some suggestions :)
84b2a12 to
119a4b4
Compare
119a4b4 to
f7cb11b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noice!
Left some comments, mostly cosmetics (other than what we discussed in person).
| log("shutdown", "info", `Received ${reason}. Cleaning up...`); | ||
| this.children.forEach((child) => { | ||
| if (!child.killed) { | ||
| child.kill(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked in person with @infomiho. We sometimes we don't sucessfully kill all the children, causing the parent process to exit without removing the container?
Let's log something when child.kill() returns false and see whether that causes the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Killing the process that started the container with docker run does not necessarily kill the Docker container. It might send a signal to the container to terminate, it might not, it depends.
More info here: https://stackoverflow.com/a/50034061
To make sure we stop the container, we could upgrade our system in couple of ways:
- run
docker stopcommand to stop the PostgreSQL container when cleaning up (It'll probably be complex since we'll be spawning new commands to clean up and then we have to wait for those to finish etc.) - use Docker API directly from Node.js via https://github.com/apocas/dockerode (I haven't looked deeper into this, but it does allow for more precise control)
I'm on the fence about what to do here, I'd keep this as simple as possible since it's our internal tool and the simplest option for me would be to just accept that fact that sometimes we'd have to manually clean up the database container. We can evolve the approach it start getting in our way. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave it like this. Please just add this explanation to the code (and possibly link to the github comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of having types files.
I'd call it utils.ts or something (it's still frowned upon, but it at least describes what the stuff inside is for).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having types.ts in various folders e.g. /types.ts and /db/types.ts and they are "useful types" used at that level of the app and deeper. I don't want to use utils.ts because that sounds to be that it might contain some runtime code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
The discussion is interesting, but you don't need to change anything about the code. Do whatever you like and resolve the thread when happy :)
I understand the intent (dependency inversion and all that) but still consider such an organization suboptimal.
I'd throw types.ts in the same bucket as values.ts, functions.ts, constants.ts, classes.ts, etc. Or, more commonly, controllers.ts, components.tsx....
It all boils down to the same principle - these names don't describe the domain, they describe the implementation details of the langauge/framework.
Types are a part of the domain like anything else, they shouldn't get special treatment. To drive the point home, if we consistently applied this naming logic to values, the entire codebase would become a bunch of modules with two files each - types.ts and values.ts.
I don't want to use utils.ts because that sounds to be that it might contain some runtime code.
I'd say that's perfectly fine 😄
Why is runtime code inside the same undesirable?
If I came up with a utility funtion (e.g, kebabToCamelCase), this is exactly where I'd like to put it. I wouldn't want two files, utils.ts and types.ts. After all, we should be organizing code by domain, not the runtime (this was the main driver behind Wasp's restructuring).
So, all in all, I think that:
src/types.tsshould be called something else. It contains utility types, so we can call itutils.ts(again, not great, but better IMO). And it's perfectly OK if one day we extend it to runtime values.src/db/types.ts- This one is tricky. In reality, it shouldn't even exist. It only exists to prevent a circular import. Maybe a circular import fromindex.tsis better than having a separate file with this type? I'm not sure, I've never found a satisfying solution to this problem. Maybe just name the fileSetupDb.ts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the domain driven file structure organisation and I quite like it. Again, the types.ts file for me has this special aura that it contains types related to the folder. I'll just keep it as-is for now since the files are really small.
3d3dfe5 to
64ffb74
Compare
|
@sodic I've decided to remove the env files management from the |
sodic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, great stuff!
I'm approving. Left some minor comments, check them out and do what you want.
wasp-app-runner/src/db/postgres.ts
Outdated
| containerName, | ||
| port, | ||
| }); | ||
| if (extraInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extraInfo is of type string | null which includes "", which is falsy.
Checking for null explicitly would probably be more clear (and possibly more correct, since I'm guessing the conditional wasn't written with an empty string in mind).
FWIW, I recommend never relying on coercion in coditionals.
To each their own, but I don't see a benefit of ever doing if (something) over if (something === ...) when something isn't a boolean. The latter is always clearer and better communicates intent IMO, while the former more often than not introduces subtle edge cases.
There's an ESLint rule for this. I'll tag @FranjoMindek and this issue (#2583) so we can reference it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you are right, we should be as strict as possible with our code 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
The discussion is interesting, but you don't need to change anything about the code. Do whatever you like and resolve the thread when happy :)
I understand the intent (dependency inversion and all that) but still consider such an organization suboptimal.
I'd throw types.ts in the same bucket as values.ts, functions.ts, constants.ts, classes.ts, etc. Or, more commonly, controllers.ts, components.tsx....
It all boils down to the same principle - these names don't describe the domain, they describe the implementation details of the langauge/framework.
Types are a part of the domain like anything else, they shouldn't get special treatment. To drive the point home, if we consistently applied this naming logic to values, the entire codebase would become a bunch of modules with two files each - types.ts and values.ts.
I don't want to use utils.ts because that sounds to be that it might contain some runtime code.
I'd say that's perfectly fine 😄
Why is runtime code inside the same undesirable?
If I came up with a utility funtion (e.g, kebabToCamelCase), this is exactly where I'd like to put it. I wouldn't want two files, utils.ts and types.ts. After all, we should be organizing code by domain, not the runtime (this was the main driver behind Wasp's restructuring).
So, all in all, I think that:
src/types.tsshould be called something else. It contains utility types, so we can call itutils.ts(again, not great, but better IMO). And it's perfectly OK if one day we extend it to runtime values.src/db/types.ts- This one is tricky. In reality, it shouldn't even exist. It only exists to prevent a circular import. Maybe a circular import fromindex.tsis better than having a separate file with this type? I'm not sure, I've never found a satisfying solution to this problem. Maybe just name the fileSetupDb.ts?
Wasp app runner is:
This will help with running headless tests on our example apps.