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

feat: Sync attachments to assets directory #254

Merged
merged 18 commits into from
Apr 10, 2024
Merged

feat: Sync attachments to assets directory #254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
08a24cb
feat: Sync attachments to assets directory
tianfeng92 Apr 4, 2024
8b2b102
refine
tianfeng92 Apr 4, 2024
3f83a02
rename env vars
tianfeng92 Apr 4, 2024
2d25e98
bump reporter
tianfeng92 Apr 4, 2024
1c43ad3
global env vars actually are applied to suites
tianfeng92 Apr 4, 2024
76f3e80
works?
tianfeng92 Apr 4, 2024
656ecdf
null?
tianfeng92 Apr 4, 2024
9a4b0c2
at least this works
tianfeng92 Apr 4, 2024
5406700
refine
tianfeng92 Apr 4, 2024
b04d3cf
refine
tianfeng92 Apr 5, 2024
97a3041
Update src/playwright-runner.ts
tianfeng92 Apr 5, 2024
e5dc69a
relocate directory setting
tianfeng92 Apr 5, 2024
470c520
fix type
tianfeng92 Apr 5, 2024
52768dd
Merge branch 'main' into DEVX-2834
tianfeng92 Apr 5, 2024
ad8c2d7
do not use empty string
tianfeng92 Apr 5, 2024
0a93d86
skip null or undefined when passing args
tianfeng92 Apr 5, 2024
7e50233
add args check condition back
tianfeng92 Apr 5, 2024
10982f3
make check clear
tianfeng92 Apr 5, 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
21 changes: 12 additions & 9 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
"homepage": "https://github.com/saucelabs/sauce-playwright-runner",
"dependencies": {
"@playwright/test": "1.41.2",
"@saucelabs/playwright-reporter": "1.0.0",
"@saucelabs/playwright-reporter": "1.1.0",
"@saucelabs/testcomposer": "2.0.0",
"dotenv": "16.4.5",
"lodash": "4.17.21",
Expand Down
18 changes: 16 additions & 2 deletions src/playwright-runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
);
return;
}
let result: any = convert.xml2js(xmlData, { compact: true });

Check warning on line 49 in src/playwright-runner.ts

View workflow job for this annotation

GitHub Actions / test 

Unexpected any. Specify a different type
if (!result.testsuites || !result.testsuites.testsuite) {
console.warn('JUnit file generation skipped: no test suites detected.');
return;
Expand All @@ -63,7 +63,7 @@
let totalSkipped = 0;
let totalTime = 0;
for (let i = 0; i < result.testsuites.testsuite.length; i++) {
const testsuite = result.testsuites.testsuite[i] as any;

Check warning on line 66 in src/playwright-runner.ts

View workflow job for this annotation

GitHub Actions / test 

Unexpected any. Specify a different type
if (testsuite._attributes) {
totalTests += +testsuite._attributes.tests || 0;
totalFailures += +testsuite._attributes.failures || 0;
Expand Down Expand Up @@ -175,7 +175,15 @@
runCfg.sauce = {};
}
runCfg.sauce.region = runCfg.sauce.region || 'us-west-1';
runCfg.playwrightOutputFolder = path.join(runCfg.assetsDir, 'test-results');
runCfg.playwrightOutputFolder =
suite.env?.SAUCE_SYNC_WEB_ASSETS?.toLowerCase() === 'true'
Copy link
Contributor

Choose a reason for hiding this comment

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

So this will only work if and only if the customer has this env var defined as the string "true"? The schema for env is a simple object so we won't be able to provide any in-editor context/autocomplete for this value.

Do we want to relax this check and just check for a non empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think set it to true is more accurate and doesn't cause further confusion like: what should I set? Let me try 1, yes, enabled and etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's your opinion @alexplischke ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not suggesting the value should only be 1 or yes or enabled. But let's say my configuration is

env:
  SAUCE_SYNC_WEB_ASSETS: enabled

Do we really want to ignore it just because its not set to exactly true? (Not a rhetorical question, honestly wondering).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then the customer might want to try with disabled to disable it, which can't work as expected.

env:
  SAUCE_SYNC_WEB_ASSETS: disabled

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course. So what would be the least surprising behaviour?

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, I don't feel that strongly one way or the other. If you think enabling if and only if set to true is better, let's go with that. We're gonna eventually transition to enabling it by default so this check is gonna be removed soon anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I think we should set the strict rules with documentation. No vague settings. But of course, this will be removed in the future so I think it's good to leave it like this 🙈

Copy link

Choose a reason for hiding this comment

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

Don't feel strongly either way as well. If you want to go the extra mile, you could do a check for 1, true, enable(d) and yes 😅 Externally we should only document the value "true".

? undefined
: path.join(runCfg.assetsDir, 'test-results');

runCfg.webAssetsDir =
suite.env?.SAUCE_SYNC_WEB_ASSETS?.toLowerCase() === 'true'
? runCfg.assetsDir
: '';

return runCfg;
}
Expand All @@ -200,7 +208,7 @@
async function run(nodeBin: string, runCfgPath: string, suiteName: string) {
const runCfg = await getCfg(runCfgPath, suiteName);

const packageInfo = require(path.join(__dirname, '..', 'package.json'));

Check warning on line 211 in src/playwright-runner.ts

View workflow job for this annotation

GitHub Actions / test 

Require statement not part of import statement
console.log(`Sauce Playwright Runner ${packageInfo.version}`);
console.log(
`Running Playwright ${packageInfo.dependencies?.playwright || ''}`,
Expand Down Expand Up @@ -303,7 +311,12 @@
// eslint-disable-next-line prefer-const
for (let [key, value] of Object.entries(args)) {
key = utils.toHyphenated(key);
if (excludeParams.includes(key.toLowerCase()) || value === false) {
if (
excludeParams.includes(key.toLowerCase()) ||
value === false ||
value === undefined ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? Because later it's going to stringify undefined 🤷‍♀️

procArgs.push(String(value));

value === null
) {
continue;
}
procArgs.push(`--${key}`);
Expand Down Expand Up @@ -332,6 +345,7 @@
PLAYWRIGHT_JUNIT_OUTPUT_NAME: runCfg.junitFile,
SAUCE_REPORT_OUTPUT_NAME: runCfg.sauceReportFile,
FORCE_COLOR: '0',
SAUCE_WEB_ASSETS_DIR: runCfg.webAssetsDir,
};

utils.setEnvironmentVariables(env);
Expand Down
4 changes: 3 additions & 1 deletion src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ export interface RunnerConfig {
preExecTimeout: number;
path: string;
projectPath: string;
playwrightOutputFolder: string;
playwrightOutputFolder?: string;
// webAssetsDir contains assets compatible with the Sauce Labs web UI.
webAssetsDir?: string;
suite: Suite;

args: Record<string, unknown>;
Expand Down
4 changes: 3 additions & 1 deletion src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ export function replaceLegacyKeys(config: SuiteConfig) {
return args;
}

export function setEnvironmentVariables(envVars: Record<string, string> = {}) {
export function setEnvironmentVariables(
envVars: Record<string, string | undefined> = {},
) {
if (!envVars) {
return;
}
Expand Down
Loading