feat: Sync attachments to assets directory #254
Conversation
Co-authored-by: Alex Plischke <[email protected]>
| if ( | ||
| excludeParams.includes(key.toLowerCase()) || | ||
| value === false || | ||
| value === undefined || |
There was a problem hiding this comment.
Why? Because later it's going to stringify undefined 🤷♀️
procArgs.push(String(value));
| @@ -175,7 +175,15 @@ async function getCfg( | |||
| 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' | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What's your opinion @alexplischke ?
There was a problem hiding this comment.
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: enabledDo we really want to ignore it just because its not set to exactly true? (Not a rhetorical question, honestly wondering).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Of course. So what would be the least surprising behaviour?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙈
There was a problem hiding this comment.
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".
Description
Control the sync assets feature using the
SAUCE_SYNC_WEB_ASSETSglobal environment variable:Feel free to suggest other env var name to replace
SAUCE_SYNC_WEB_ASSETS.Job example: https://app.saucelabs.com/tests/b7e69e6cc94f4616bec8c5f05b7a3824