Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Replace karma and testswarm with jQuery test runner; add filestash workflow #503

Merged
merged 2 commits into from
Mar 20, 2024
Merged

Replace karma and testswarm with jQuery test runner; add filestash workflow #503

merged 2 commits into from
Mar 20, 2024

Conversation

timmywil
Copy link
Member

@timmywil timmywil commented Mar 18, 2024 •
edited
Loading

  • replaces karma and testswarm with the new jQuery test runner
  • 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.
  • intentionally running both dev and min on every supported jQuery version. Before, swarm was only running min on certain versions, but this was actually easier to support and didn't add much time to the test runs.
  • environment variables are already in place
  • upgrade all dependencies, but lock uglify to a version that supports the 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

@timmywil timmywil requested a review from mgol March 18, 2024 15:14
@timmywil
Tests: replace karma and testswarm with jQuery test runner
2d4424e 
- 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.
mgol
mgol reviewed Mar 18, 2024
View reviewed changes
Copy link
Member

@mgol mgol left a 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!

.github/workflows/browserstack.yml Outdated Show resolved Hide resolved
.github/workflows/browserstack.yml Outdated Show resolved Hide resolved
.github/workflows/node.js.yml Outdated Show resolved Hide resolved
Gruntfile.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/runner/command.js Show resolved Hide resolved
test/runner/command.js
} else if ( argv.browserstackPlan ) {
console.log( await getPlan() );
} else {
run( argv );
Copy link
Member

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?

Copy link
Member Author

@timmywil timmywil Mar 19, 2024 •
edited
Loading

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.

Copy link
Member

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.

test/runner/createTestServer.js Outdated Show resolved Hide resolved
test/runner/package.json Outdated Show resolved Hide resolved
test/runner/lib/buildTestUrl.js Show resolved Hide resolved
timmywil
timmywil commented Mar 19, 2024
View reviewed changes
src/jquery/css.js
@@ -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
Copy link
Member Author

@timmywil timmywil Mar 19, 2024 •
edited
Loading

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.

@timmywil
Copy link
Member Author

timmywil commented Mar 19, 2024 •
edited
Loading

New sample BrowserStack runs:

3.x: https://github.com/timmywil/jquery-migrate/actions/runs/8345191538
git: https://github.com/timmywil/jquery-migrate/actions/runs/8345191528

@timmywil timmywil requested a review from mgol March 19, 2024 18:05
mgol
mgol approved these changes Mar 19, 2024
View reviewed changes
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@mgol
Copy link
Member

mgol commented Mar 20, 2024

@timmywil I disabled both Jenkins Migrate jobs. Feel free to merge the PR.

@mgol mgol added this to the 3.5.0 milestone Mar 20, 2024
@timmywil timmywil merged commit f3b98e6 into jquery:main Mar 20, 2024
10 checks passed
@timmywil timmywil deleted the test branch March 20, 2024 13:48
@mgol mgol modified the milestones: 3.5.0, 3.4.2 Jul 8, 2024
mgol added a commit to mgol/jquery-migrate that referenced this pull request Jul 10, 2024
@mgol
Release: Fix the release script
5218ff2 
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`.
@mgol mgol mentioned this pull request Jul 10, 2024
Merged
mgol added a commit to mgol/jquery-migrate that referenced this pull request Jul 10, 2024
@mgol
Release: Fix the release script
7825856 
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
mgol added a commit that referenced this pull request Jul 10, 2024
@mgol
Release: Fix the release script
ffa305f 
PR gh-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`.

Fixes gh-521
Ref gh-503
This was referenced Aug 4, 2024
Closed
Closed
Closed
@alikon alikon mentioned this pull request Aug 11, 2024
Closed
@matthewluallen matthewluallen mentioned this pull request Sep 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgol mgol mgol approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.
2 participants
@timmywil @mgol