-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Changes from 6 commits
363ace4
4dc292f
1a37f3f
0979736
7be4d22
3661b44
32a1987
2cb4950
de3ae93
4aafaef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
e2e/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
package-lock = false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { defineConfig } from 'eslint/config'; | ||
|
||
import eslintPlugin from 'eslint-plugin-eslint-plugin'; | ||
|
||
export default defineConfig([ | ||
{ | ||
linterOptions: { | ||
reportUnusedDisableDirectives: 'error', | ||
}, | ||
}, | ||
{ | ||
extends: [eslintPlugin.configs.all], | ||
files: ['./index.js', './rule.js'], | ||
}, | ||
]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import rule from './rule.js'; | ||
|
||
/** @type {import('eslint').ESLint.Plugin} */ | ||
const plugin = { | ||
meta: { | ||
name: '@e2e/all-typed-config', | ||
version: '1.0.0', | ||
}, | ||
rules: { 'e2e-test': rule }, | ||
configs: { | ||
recommended: { | ||
name: '@e2e/all-typed-config/recommended', | ||
plugins: { | ||
get ['@e2e/all-typed-config']() { | ||
return plugin; | ||
}, | ||
}, | ||
rules: { | ||
'@e2e/all-typed-config/e2e-test': 'error', | ||
}, | ||
}, | ||
}, | ||
}; | ||
|
||
export default plugin; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
{ | ||
"name": "@e2e/all-typed-config", | ||
"private": true, | ||
"type": "module", | ||
"scripts": { | ||
"lint": "tsc && eslint" | ||
}, | ||
"main": "./index.js", | ||
"devDependencies": { | ||
"eslint": "^9.33.0", | ||
"eslint-plugin-eslint-plugin": "file:../..", | ||
"jiti": "^2.5.1", | ||
"typescript": "^5.9.2" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/** @type {import('eslint').Rule.RuleModule} */ | ||
const rule = { | ||
// eslint-disable-next-line eslint-plugin/require-meta-schema | ||
meta: { | ||
type: 'suggestion', | ||
docs: { | ||
description: 'enforce a test', | ||
recommended: false, | ||
url: 'https://test.org', | ||
}, | ||
fixable: undefined, // or "code" or "whitespace" | ||
messages: { | ||
missingOutput: 'This is a test', | ||
}, | ||
}, | ||
|
||
create(context) { | ||
return { | ||
Program(ast) { | ||
const sourceCode = context.sourceCode; | ||
if (false) { | ||
context.report({ | ||
node: ast, | ||
messageId: 'missingOutput', | ||
}); | ||
} | ||
}, | ||
}; | ||
}, | ||
}; | ||
|
||
export default rule; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
{ | ||
"compilerOptions": { | ||
"rootDir": ".", | ||
"module": "nodenext", | ||
"moduleResolution": "nodenext", | ||
"noEmit": true, | ||
"skipLibCheck": true, | ||
"strict": true, | ||
"target": "ES2024", | ||
"verbatimModuleSyntax": true, | ||
"erasableSyntaxOnly": true, | ||
"types": [] | ||
}, | ||
"include": ["eslint.config.ts"] | ||
} |
michaelfaith marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
package-lock = false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { defineConfig } from 'eslint/config'; | ||
|
||
import eslintPlugin from 'eslint-plugin-eslint-plugin'; | ||
|
||
export default defineConfig([ | ||
{ | ||
linterOptions: { | ||
reportUnusedDisableDirectives: 'error', | ||
}, | ||
}, | ||
{ | ||
extends: [eslintPlugin.configs.all], | ||
files: ['./index.js', './rule.js'], | ||
}, | ||
]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import rule from './rule.js'; | ||
|
||
/** @type {import('eslint').ESLint.Plugin} */ | ||
const plugin = { | ||
meta: { | ||
name: '@e2e/all', | ||
version: '1.0.0', | ||
}, | ||
rules: { 'e2e-test': rule }, | ||
configs: { | ||
recommended: { | ||
name: '@e2e/all/recommended', | ||
plugins: { | ||
get ['@e2e/all']() { | ||
return plugin; | ||
}, | ||
}, | ||
rules: { | ||
'@e2e/all/e2e-test': 'error', | ||
}, | ||
}, | ||
}, | ||
}; | ||
|
||
export default plugin; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
{ | ||
"name": "@e2e/all", | ||
"private": true, | ||
"type": "module", | ||
"scripts": { | ||
"lint": "eslint" | ||
}, | ||
"main": "./index.js", | ||
"devDependencies": { | ||
"eslint": "^9.33.0", | ||
"eslint-plugin-eslint-plugin": "file:../.." | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/** @type {import('eslint').Rule.RuleModule} */ | ||
const rule = { | ||
// eslint-disable-next-line eslint-plugin/require-meta-schema | ||
meta: { | ||
type: 'suggestion', | ||
docs: { | ||
description: 'enforce a test', | ||
recommended: false, | ||
url: 'https://test.org', | ||
}, | ||
fixable: undefined, // or "code" or "whitespace" | ||
messages: { | ||
missingOutput: 'This is a test', | ||
}, | ||
}, | ||
|
||
create(context) { | ||
return { | ||
Program(ast) { | ||
const sourceCode = context.sourceCode; | ||
if (false) { | ||
context.report({ | ||
node: ast, | ||
messageId: 'missingOutput', | ||
}); | ||
} | ||
}, | ||
}; | ||
}, | ||
}; | ||
|
||
export default rule; |
michaelfaith marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
import { execSync } from 'node:child_process'; | ||
import fs from 'node:fs/promises'; | ||
import path from 'node:path'; | ||
import process from 'node:process'; | ||
|
||
/** | ||
* 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 commentThe 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 commentThe 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 commentThe 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. |
||
* | ||
* For each directory under fixtures, the script runs `npm install` and `npm run lint`. | ||
*/ | ||
|
||
const TEST_COMMAND = 'npm run lint'; | ||
|
||
const getRoot = () => { | ||
return execSync('git rev-parse --show-toplevel', { | ||
encoding: 'utf-8', | ||
}).trim(); | ||
}; | ||
|
||
const executeAllE2eTests = async () => { | ||
const e2eDir = path.resolve(getRoot(), './e2e'); | ||
const failedTests = []; | ||
|
||
// Get all directories in the e2e dir | ||
const testDirs = (await fs.readdir(e2eDir, { withFileTypes: true })) | ||
.filter((dirEnt) => dirEnt.isDirectory()) | ||
.map((dirEnt) => path.join(dirEnt.parentPath, dirEnt.name)); | ||
|
||
for (const testDir of testDirs) { | ||
const dirName = path.basename(testDir); | ||
console.log(`🧪 Executing test: ${dirName}`); | ||
|
||
// Run install and the test command | ||
execSync('npm install', { | ||
cwd: testDir, | ||
stdio: ['ignore', 'ignore', 'pipe'], | ||
}); | ||
try { | ||
execSync(TEST_COMMAND, { | ||
cwd: testDir, | ||
stdio: 'inherit', | ||
}); | ||
console.log(`✅ Test passed`); | ||
} catch (error) { | ||
console.log(`❌ Test failed`); | ||
failedTests.push(dirName); | ||
} | ||
console.log(`\n${'-'.repeat(50)}\n`); | ||
} | ||
|
||
if (failedTests.length) { | ||
console.log( | ||
`Testing complete. ${failedTests.length} of ${testDirs.length} tests failed!`, | ||
); | ||
process.exitCode = 1; | ||
} else { | ||
console.log(`Testing complete. All ${testDirs.length} tests passed!`); | ||
} | ||
}; | ||
|
||
executeAllE2eTests(); |
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?
Uh oh!
There was an error while loading. Please reload this page.
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.