Skip to content

ci: updated ci workflow and remove appveyor #192

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

Merged
merged 1 commit into from
Mar 23, 2025

Conversation

Phillip9587
Copy link
Contributor

@Phillip9587 Phillip9587 commented Feb 6, 2025

I noticed that the current CI workflow in this repository could benefit from some updates. Specifically:

  1. The Coverage setup could also be optimized:

    • Currently, the workflow uses the coverallsapp/github-action@master which points to v1 of this action. This v1 action uses node16 as runtime which is deprecated.
  2. Minimum token permissions for the GITHUB_TOKEN:

  3. lint step

    • I created an extra lint step similar to the workflow of the main express repo.
  4. Test Matrix

    • I added Node.js v19, v21 and v23 to the test matrix to test all supported Node.js versions.
    • I added windows-latest and ubuntu-latest to the test matrix to test on both
  5. Removed obsolete AppVeyor

  • Removed obsolete appveyor.yml file
  • Removed the appveyor badge from README.md

closes #193

@bjohansebas
Copy link
Member

Please remove AppVeyor and add testing on Windows, maybe macos?

@Phillip9587 Phillip9587 changed the title ci: updated ci workflow ci: updated ci workflow and remove appveyor Feb 14, 2025
@Phillip9587
Copy link
Contributor Author

@bjohansebas I removed Appveyor and integrated windows testing into the github actions ci workflow

@Phillip9587
Copy link
Contributor Author

@bjohansebas I think adding macos-latest to the test matrix should happen in another PR as it needs to be discussed if we want to test on macos because even the main express repos does not run its tests on macos.

Copy link
Member

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

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

LGTM

@UlisesGascon UlisesGascon merged commit 55475c7 into expressjs:master Mar 23, 2025
18 checks passed
@UlisesGascon UlisesGascon mentioned this pull request Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove AppVeyor Configuration and Integrate Windows Testing in GitHub Actions
4 participants