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

[engsys] improve smoke test runner #20872

Merged
merged 3 commits into from
Mar 30, 2022
Merged

[engsys] improve smoke test runner #20872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ac6a70
[engsys] improve smoke test runner
jeremymeng Mar 17, 2022
6f38d17
remove exitCode variable
jeremymeng Mar 17, 2022
1fc0d5c
Don't skip arm-* samples here.
jeremymeng Mar 30, 2022
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
55 changes: 25 additions & 30 deletions common/smoke-test/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@ async function main() {
// Read manifest
const manifest = require("./run-manifest.json");

let exitCode = 0;
let samplesToExecute = [];
let failures = [];
const samplesToExecute = [];
const failures = [];

// Bring all samples and includes into memory
for (let entry of manifest) {
console.log(`Importing samples for ${entry.Name}...`);
for (const entry of manifest) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we include a TODO here to start running the ARM samples as well at some point?

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 wasn't sure about arm- samples. @praveenkuttappan do you know whether we want them to run as part of smoke tests

Copy link
Member Author

Choose a reason for hiding this comment

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

@ckairen should we exclude ARM samples when generating the run manifest json, or is it also desirable to include them?

Copy link
Member

Choose a reason for hiding this comment

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

I think it will be beneficial for smoke test

console.log(`Gathering samples for ${entry.Name}...`);

// Read configuration from package.json's //sampleConfiguration field
const packageJson = require(`${entry.PackageDirectory}/package.json`);
Expand All @@ -22,71 +21,67 @@ async function main() {
}

const skipFiles = smokeTestConfig.skip || [];
const jsFiles = fs.readdirSync(entry.SamplesDirectory)
const jsFiles = fs
.readdirSync(entry.SamplesDirectory)
.filter((name) => name.endsWith(".js"))
.filter((name) => !skipFiles.includes(name));

for (let targetSample of jsFiles) {
const sampleModule = require(`${entry.SamplesDirectory}/${targetSample}`);
for (const targetSample of jsFiles) {
samplesToExecute.push({
entrypoint: sampleModule.main,
name: entry.Name,
sampleFile: targetSample,
directory: entry.SamplesDirectory
directory: entry.SamplesDirectory,
});
}
}

// Run all samples
for (let sample of samplesToExecute) {
let result = await executeSample(sample);
for (const sample of samplesToExecute) {
const result = await executeSample(sample);

if (!result.success) {
exitCode = 1;
failures.push({
sample,
result
result,
});
}
}

if (failures.length > 0) {
console.log("SMOKE TEST FAILURES");
for (let failure of failures) {
for (const failure of failures) {
console.error(
`Test Failed - Package: ${failure.sample.name} - Sample File:${failure.sample.sampleFile}`
);
console.log(`Exception: ${failure.result.exception}`);
console.log(failure);
}
process.exit(1);
}

// TODO: Don't do it this way if possible?
process.exit(exitCode);
process.exit();
}

async function executeSample(sample) {
const { name, sampleFile, directory } = sample;
console.log("============== SMOKE TESTS ==============");
console.log(`Sample Name: ${sample.name}`);
console.log(`Sample File: ${sample.sampleFile}`);
console.log(` Sample Name: ${name}`);
console.log(` Sample File: ${sampleFile}`);

let result = {
success: true
};

let currentDir = process.cwd();
const currentDir = process.cwd();

try {
// Set the process directory to the sample's directory because some samples
// use file paths relative to the sample's directory.
process.chdir(sample.directory);

await sample.entrypoint();
process.chdir(directory);
const entryPoint = require(`${directory}/${sampleFile}`).main;
await entryPoint();
} catch (exception) {
console.log("FAILURE");
result = {
console.log(" FAILURE");
return {
success: false,
exception
exception,
};
} finally {
// Reset the working directory to the root directory after execution
Expand All @@ -95,7 +90,7 @@ async function executeSample(sample) {

console.log("=========================================");

return result;
return { success: true };
}

// If command line parameter `--devops-logging` is set, then have console.error
Expand Down
2 changes: 1 addition & 1 deletion sdk/eventgrid/eventgrid/samples-dev/consumeEventsFromServiceBusQueue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ async function main() {
// Stop processing events and exit.
await closer.close();
await receiver.close();
process.exit();
}

main().catch((err) => {
console.error("The sample encountered an error:", err);
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering - would it be better if we rethrew the error or something to allow the harness to know this sample failed but other scenarios could continue to run?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! This actually doesn't matter to the test runner as it only invokes the sample's main() function. I was just following the convention of most existing samples.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible that process.exit(1) was used at some point? @witemple-msft I wonder whether you have any ideas as you worked with dev-tool and samples before?

Copy link
Member

Choose a reason for hiding this comment

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

This is just a conventional pattern for async scripts in JavaScript. Instead of rethrowing the error, you also exit(1). The problem, fundamentally, is that an unhandled Promise rejection does not result in an error status in some (all?) versions of Node, so explicitly calling exit is the only way for the script itself to report that it failed to the shell.

});
2 changes: 1 addition & 1 deletion sdk/eventgrid/eventgrid/samples/v4/javascript/consumeEventsFromServiceBusQueue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ async function main() {
// Stop processing events and exit.
await closer.close();
await receiver.close();
process.exit();
}

main().catch((err) => {
console.error("The sample encountered an error:", err);
process.exit(1);
});
2 changes: 1 addition & 1 deletion sdk/eventgrid/eventgrid/samples/v4/typescript/src/consumeEventsFromServiceBusQueue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ async function main() {
// Stop processing events and exit.
await closer.close();
await receiver.close();
process.exit();
}

main().catch((err) => {
console.error("The sample encountered an error:", err);
process.exit(1);
});