-
Notifications
You must be signed in to change notification settings - Fork 471
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
Replace karma and testswarm with jQuery test runner; add filestash workflow #503
Conversation
- add GH workflow for updating git files on filestash - selenium is having an issue starting safaridriver when loading esmodules. Retries succeed, but inconsistently. Leaving that out for now.
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.
A few comments. Can't wait to see it in Migrate!
} else if ( argv.browserstackPlan ) { | ||
console.log( await getPlan() ); | ||
} else { | ||
run( argv ); |
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.
In Core, you had:
run( {
...argv,
browsers: argv.browser,
modules: argv.module
} );
Is that not needed here? If so, why?
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.
With the extra options (jquery
and jqueryMigrate
) also getting pluralized, I moved the pluralization to the run parameters instead so the function call in the command can be agnostic. I could do the same in core, but it became more necessary with the extra arguments.
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 see, makes sense. Maybe you could also do it in Core later for consistency.
@@ -81,7 +81,7 @@ if ( jQueryVersionSince( "3.4.0" ) && typeof Proxy !== "undefined" ) { | |||
} | |||
|
|||
// In jQuery >=4 where jQuery.cssNumber is missing fill it with the latest 3.x version: | |||
// https://github.com/jquery/jquery/blob/3.7.0/src/css.js#L216-L246 | |||
// https://github.com/jquery/jquery/blob/3.7.1/src/css.js#L216-L246 |
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 did check the link here and the lines are the same.
New sample BrowserStack runs: 3.x: https://github.com/timmywil/jquery-migrate/actions/runs/8345191538 |
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.
Looks great, thanks!
@timmywil I disabled both Jenkins Migrate jobs. Feel free to merge the PR. |
PR jquerygh-503 erroneously removed the `enquirer` dependency, required for the release process. Restore it. In addition, update built-in module imports to use the `node:` prefix and add a missing "utf8" value in one `fs.readFileSync`.
PR jquerygh-503 erroneously removed the `enquirer` dependency, required for the release process. Restore it. In addition, update built-in module imports to use the `node:` prefix and add a missing "utf8" value in one `fs.readFileSync`. Ref jquerygh-503
ie8
argument.Disable jenkins job before merging
Sample runs:
Filestash dry run: https://github.com/timmywil/jquery-migrate/actions/runs/8328770535/job/22789518882
Browserstack: https://github.com/timmywil/jquery-migrate/actions/runs/8328770533
Browserstack (git): https://github.com/timmywil/jquery-migrate/actions/runs/8328770540