-
Notifications
You must be signed in to change notification settings - Fork 14
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
Update service to ESM and WDIO v8 #126
base: main
Are you sure you want to change the base?
Conversation
Updated to WDIO v8 Updated to Node v18
Switch to jsconfig for now Updated necessary types for spec files Specify browserVersion for wdio conf
Replaced import all with default import Removed unnecessary export
@dons20 thanks for taking a stab at it. Happy to help if needed. |
Haven't forgotten about this one, I had some major PC issues. SSD failed but I got it replaced. Will have some changes out soon again. |
Removed older babel references, configs Added types in place of JSDoc types
Update files to correct line-endings
Hey @dons20 , thanks for all your work so far! Let me know when you think this is ready for review! |
Will do, I only have to retest the test directory with these new changes. Everything else should be done |
Updated deps Added temporary file for further run arg types and comments
Back at it, pushed some of my changes, and I'll have some more incoming soon. Trying to get the local build and tests working fully. |
Awesome, looking forward to the changes. Let me know when I can review something. |
Added mocks where needed Updated tsconfigs to handle build vs development type-checking Updated imports where applicable Other minor improvements
Updated more configs Added some dev dependencies
I'm pretty happy with the results so far, and I think most of this can start to be reviewed, but you can let me know if I should take this out of draft yet. I still have these items on my todo list:
|
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.
Some comments, overall looks aweseome!
} | ||
} | ||
|
||
class DockerLauncherForTests extends DockerLauncher { |
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 is this needed, seems like DockerLauncher
defines this props to be public
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.
When writing the tests, it complained about some variables being private or protected when trying to stub or spy. Perhaps there's a better way of resolving this?
startDelay?: number; | ||
}; | ||
|
||
export type DockerRunArgs = { |
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.
Is there an NPM package that contains these types?
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.
Not that I'm aware of unfortunately. If a good, updated one exists, I'd be happy to migrate to it.
@@ -0,0 +1,9 @@ | |||
import fetch from 'node-fetch'; | |||
|
|||
class PingClass { |
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.
Are your planning to add anything to this class? If not I guess it makes sense to not abstract a single fetch
call
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.
This one I'm not particularly opinionated about. It was previously just a regular function export but I migrated it to a class to address a TS issue in Sinon ES Modules cannot be stubbed
. I was using one of the suggestions listed here.
Thanks for reviewing! Will go through the comments shortly |
@dons20 any updates or anything I can help with? |
No updates yet, had a couple of busy weeks but I will make some updates in the coming days 🤞🏾 |
Would it help if I separated out the upgrade from v7 to v8 to fix the CI pipeline added in #132? |
@seanpoulter If that is something you can manage in a short period of time, then feel free to |
It's probably no surprise but the update from v7 to v8 requires loading the ES Module Let me know if there's anything else I can do to help. |
Totally understand. I'm pushing some changes within this weekend, hopefully it will advance this PR much further so it can cover all the concerns. |
More usage of sandbox instead of direct stubs
Removed console log Fixed some warnings and errors displayed in some test contexts
So recently I've been working on the unit tests as they were tricky to get started. After much debugging I had realized that one file was blocking the others from executing. Once I commented some of those tests, the others started running and I was able to fix the failing ones. There are about 20 to resolve on the unit tests side. I'll continue chipping away at it in the coming days whenever I can find time. Just keeping everyone in the loop. |
Description
This started as a small experiment to make some changes locally to update the service. I figured it may be a good idea to update this to use ESM as well as Node v20 since it's the latest LTS at this time.
Leaving this up as a draft to get some initial thoughts. I'm not particularly opinionated as to the direction this should go either. Will need to do additional testing so that everything is compatible, but hopefully some of these changes help 🙂
Checklist