-
Notifications
You must be signed in to change notification settings - Fork 37
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
Move all tests over to electron #4484
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
I think https://github.com/KittyCAD/modeling-app/actions/runs/11845816046/job/33014820216?pr=4484 is indicating we have 35 failures we've always been missing in our electron app. I'll correct them as part of this transition. It could take some time |
@nadr0 would you know why it says "expected..." ? Is it because we have them as required in settings and GH doesn't know this when they're removed? |
618fffc
to
1bd5dc2
Compare
@@ -221,136 +216,3 @@ jobs: | |||
retention-days: 30 | |||
overwrite: true | |||
|
|||
|
|||
electron: |
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.
Why did we delete this electron configuration and make the web one above the new electron configuration?
If we are deprecating web, we should have used the electron configuration that already runs and works.
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.
They were 99% the same 🤷
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.
My only concern is that we completely deleted the electron .yml configuration and then update the browser configuration to be electron.
I looked at them both side by side but more reviewers is always welcomed
@@ -0,0 +1,37 @@ | |||
#!/bin/sh |
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.
Do we need this file?
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.
Nope intended on removing it
Okay I am caught up on this work, I see that we are deprecating browser and moving it to electron. My only concern is that we completely deleted the I would have to diff both of those configurations to see if there is anything subtle between the two in the pipeline job. My gut reaction would be completely delete browser and leave electron as the base because that is the one we already have running. |
The electron tests are able to cover both web and desktop variants of the application. This PR moves any web-only tests over to electron.
The motivation for this is there are aspects of the application that interact with the external system and can affect the application. Our tests completely miss these effects, and cause incidents.