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

[kbn-test] improve check_ftr_configs script #189357

Merged
merged 2 commits into from
Jul 29, 2024
Merged

[kbn-test] improve check_ftr_configs script #189357

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba48a64
[kbn-test] improve check_ftr_configs script
dmlemeshko Jul 29, 2024
1b16bab
Merge branch 'main' into kbn-test/speedup-configs-checker
dmlemeshko Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions packages/kbn-test/src/functional_test_runner/lib/config/run_check_ftr_configs_cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ export async function runCheckFtrConfigsCli() {
.split('\n')
.map((file) => Path.resolve(REPO_ROOT, file));

const loadingConfigs = [];

const possibleConfigs = files.filter((file) => {
if (IGNORED_PATHS.includes(file)) {
return false;
Expand All @@ -66,24 +68,45 @@ export async function runCheckFtrConfigsCli() {
return false;
}

if (file.match(/mocks.ts$/)) {
// No FTR configs in /packages/
if (file.match(/\/packages\//)) {
return false;
}

if (file.match(/(mock|mocks).ts$/)) {
return false;
}

const fileContent = readFileSync(file).toString();

if (fileContent.match(/(testRunner)|(testFiles)/)) {
if (
// explicitly define 'testRunner' or 'testFiles'
fileContent.match(/(testRunner)|(testFiles)/) ||
// export default createTestConfig
fileContent.match(/export\s+default\s+createTestConfig/) ||
// export default async function ({ readConfigFile }: FtrConfigProviderContext)
Copy link
Member

Choose a reason for hiding this comment

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

Do you wanna drop these comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would keep it so the next person working on it have an idea of what text we are searching in files. Maybe rephrase it?

Copy link
Member

Choose a reason for hiding this comment

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

Um, what do you mean?
Sounds like I'm missing something bud.

Copy link
Member Author

Choose a reason for hiding this comment

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

fileContent is a string with js/ts file content and we use regexp to find something that might tell if it is FTR config.
It is easy see an example that you can search repo with to find if some files still has the string, e.g. export default createTestConfig

Copy link
Member

Choose a reason for hiding this comment

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

ah, nvm I get it.

// async function config({ readConfigFile }: FtrConfigProviderContext)
// export default async function (ftrConfigProviderContext: FtrConfigProviderContext)
fileContent.match(
/(?:export\s+default\s+)?async\s+function(?:\s+\w+)?\s*\(\s*(?:\{\s*readConfigFile\s*\}|\w+)\s*(?::\s*FtrConfigProviderContext\s*)?\)/
Copy link
Member

Choose a reason for hiding this comment

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

Can we name this? That's a lot of reading lol

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I can put it into variable with comment

Copy link
Member

Choose a reason for hiding this comment

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

that would be better than reading 115 characters lol

)
) {
// test config
return true;
}

if (fileContent.match(/(describe)|(defineCypressConfig)/)) {
if (file.match(/config.ts$/) && fileContent.match(/export\s+default\s+configs\./)) {
return true;
}

if (fileContent.match(/(describe)|(defineCypressConfig)|(cy\.)/)) {
// test file or Cypress config
return false;
}

// FTR config file should have default export
try {
loadingConfigs.push(file);
// eslint-disable-next-line @typescript-eslint/no-var-requires
const exports = require(file);
const defaultExport = exports.__esModule ? exports.default : exports;
Expand All @@ -94,6 +117,10 @@ export async function runCheckFtrConfigsCli() {
}
});

if (loadingConfigs.length) {
log.info(`${loadingConfigs.length} files were loaded as FTR configs for validation`);
}

const { allFtrConfigs, manifestPaths } = getAllFtrConfigsAndManifests();

const invalid = possibleConfigs.filter((path) => !allFtrConfigs.includes(path));
Expand Down