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.

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

New Integration tests (example split continuation) #5492

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

UlisesGascon
Copy link
Member

@UlisesGascon UlisesGascon commented Feb 18, 2024

Main Changes

This PR introduces a new pipeline .github/workflows/integration-tests.yml.

  • Any PR or push will trigger this pipeline, that will execute the .github/workflows/ci.yml in expressjs/examples.
  • We select the branch in scope and the repo, depending if it is a PR from a fork or a PR from the express repo itself.
  • This PR requires a PAT Token to work, see. I am currently using PAT scoped token for UlisesGascon/express-examples

Important

While this PRs triggers the process, does not wait until the .github/workflows/ci.yml is completed (fail or success) and also does not provide feedback, for example the action executed URL or similar. I think that this PR is a good step in the right direction but not yet enough in terms of DX

Here you can find some screenshots on how it works

From Express fork:
Screenshot 2024-02-18 at 18 26 54

Screenshot 2024-02-18 at 18 26 59

From Examples fork:

Screenshot 2024-02-18 at 18 26 06

Screenshot 2024-02-18 at 18 07 03

Context

This PR is supported by UlisesGascon/express-examples#7 and UlisesGascon/express-examples#8

Notes

  • This PR is polluted by Split examples from Express Core #5311 as it is a continuation
  • The pipeline .github/workflows/integration-tests.yml is the only thing in scope in this PR
  • The pipeline .github/workflows/integration-tests.yml will fail until the repo ulisesgascon/express-examples is migrated to expressjs/examples

Changelog

@UlisesGascon UlisesGascon changed the title New Integration tests (example slip continuation) New Integration tests (example split continuation) Feb 18, 2024
@UlisesGascon
Copy link
Member Author

With the changes on 41cecb0, I was able to remove dependencies and monitor the execution details for the examples repo (Note that I tested it between my forks).

In the logs the URL of the executed pipeline is included so it is much easier to follow.

Failed case

Screenshot from 2024-04-18 17-19-06
Screenshot from 2024-04-18 17-18-49

Success case

Screenshot from 2024-04-18 17-22-27

Screenshot from 2024-04-18 17-22-36

Other cases

I added a timeout to 10m (harcoded) in case that there are any infra issues but I was not able to simulate that error.

@UlisesGascon
Copy link
Member Author

Some notes that might require us to fine-tune this decision (cc: @expressjs/express-tc and Captains).

As it stands, the idea is to remove the examples from the Express core and keep them as a separate repository at https://github.com/expressjs/examples so they can evolve independently (ES6, latest versions, etc.).

Currently, the examples are used as part of the testing process, so if we separate them, we'll lose the capability to run them locally. Additionally, if we look at the fork (https://github.com/UlisesGascon/express-examples/blob/main/.github/workflows/ci.yml), the idea is to use the newer versions of Node.js, which are more aligned with Express@5 (see: #5595).

So, what do you think?

  • Should we keep the examples in v4 and only use them in v5? Or should we make the effort to have two branches (4.x and 5.x) in the examples repo so we can run the tests against one or the other based on our expectations?
  • Is it okay to ignore the end-to-end tests while developing locally and review the impact of our changes only in the CI? Should we include a script to fetch them, set up the environment, and then allow us to run the tests locally?

@UlisesGascon UlisesGascon added help wanted feedback semver-ignore This change does not have any impact in semver (docs, tooling, etc..) labels Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback help wanted semver-ignore This change does not have any impact in semver (docs, tooling, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants