-
-
Notifications
You must be signed in to change notification settings - Fork 33
ci: add e2e tests to CI #546
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
Conversation
This change adds a new end-to-end test capability. There are two new jobs in the main CI workflow. One job to build the package (which also good to have outside of the need to run e2e tests), executed with the latest LTS node. The build artifact is uploaded for the downstream `e2e` job to use. The `e2e` job, downloads the artifact, run `npm install` and then `npm run e2e`, which executes a custom node script to loop through subdirectories of `/e2e`, which each contain a project fixture with different setups. Both projects have the same simple plugin source code, and each has this plugin (referenced from the root of the repo (aka the result of the build)) and `eslint` installed as `devDependencies`. Each also has a `lint` package.json script that runs `eslint` on the simple plugin. \## `/e2e/all` This fixture has a regular js config and runs our `all` config on the project. \## `/e2e/all-typed-config` This fixture has `typescript` installed and uses a ts-based config and runs our `all` config on the project. It also executes `tsc` as part of the `lint` command, to ensure everything's typed property.
bmish
left a comment
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.
Neat.
I also ran this through AI if you'd like to have a look at what it thought: https://chatgpt.com/share/68993709-d248-800c-a59f-c765c9b7b043
| - '20.19.0' #minimum supported v20 | ||
| - 20 | ||
| os: | ||
| - ubuntu-latest |
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 like we may be missing tests on Windows. Should we test that here?
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.
Sure, I can add that. Do you want it added to the unit test matrix too?
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.
Added to the e2e job. If you want me to add it to the test job, I can do that too.
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.
Generally would be good to test on both. Hopefully it doesn't hurt perf too much. Also could be done separately.
| /** | ||
| * Run All Tests: This script executes the lint command on all subfolders under `fixtures`, in order | ||
| * to validate the correctness of our plugin. Each fixture installs the *built* package. So this should | ||
| * only be run after a build has been done. |
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.
Out of curiosity, how is the performance impact of this on our builds?
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 total, it adds around 40s to a minute to the overall CI runtime. Mainly because the E2E steps have to wait for the build to complete. Just looking at the build readout below, it took 38s for the build job, which ran in parallel to all of the other existing jobs. At that point all the E2E jobs kicked off in parallel, and the longest running of those are the windows ones, which took about a minute each (the ubuntu ones took between 40-50s).
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.
RE: performance, though, there will be a roughly linear impact to the length of those steps, for each e2e test that's added in the future. So, one thing to consider, if many more are added, would be to run some number of them concurrently. I wouldn't necessarily go to that right now, because it adds some amount of complexity. With just two tests, there's not much to gain with doing them concurrently. But if there were 10 or more, it might start to make sense.
|
Btw, I'm thinking it might be a good idea to move the prettier config back to js, until the VSCode extension supports it. prettier/prettier-vscode#3623 IDE-autoformatting isn't working anymore. What do you think? |
bmish
left a comment
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.
Thanks!
Up to you on that. If it's going to be broken for a while, then could make sense to revert. If it's going to be fixed soonish, then could wait. |
|
Is there a way to simplify this process? This change introduces 10+ jobs, and I wonder if it's really necessary - although grateful for Microsoft's generosity in offering it for free to open-source projects, I still prefer to minimize the resource consumption. |
|
Yeah, it's 12: 6 node versions * two OSs. Node Matrix
OS Matrix
So, I mean, the only way to reduce that is to remove some from those matrices, which would mean you're not testing the full range of versions that the library claims to support. |
|
We already have some compatibility tests related to Node versions, such as n/no-unsupported-features/node-builtins. Moreover, based on my experience maintaining this package, we have hardly received any issues related to Node.js/OS compatibility. So I feel like we're paying a little too many resources on things with a very low probability. 😄 |
|
Idk, there's been a lot of changes recently to node that directly affect tooling (e.g. require(ESM), type stripping, etc). And so, I think testing the various versions of node is important, since it can avoid blind spots. RE: OSes, I know that we've had issues with Windows compatibility on some of our stuff in |
|
right, you sold me! |

This change adds a new end-to-end test capability. There are two new jobs in the main CI workflow. One job to build the package (which is also good to have outside of the need to run e2e tests), executed with the latest LTS node. The build artifact is uploaded for the downstream
e2ejob to use. Thee2ejob, downloads the artifact, runnpm installand thennpm run e2e, which executes a custom node script to loop through subdirectories of/e2e, which each contain a project fixture with different setups. Both projects have the same simple plugin source code, and each has this plugin (referenced from the root of the repo (aka the result of the build)) andeslintinstalled asdevDependencies. Each also has alintpackage.json script that runseslinton the simple plugin./e2e/allThis fixture has a regular js config and runs our
allconfig on the project./e2e/all-typed-configThis fixture has
typescriptinstalled and uses a ts-based config and runs ourallconfig on the project. It also executestscas part of thelintcommand, to ensure everything's typed property.