Fix/tmp dir build #16859
Fix/tmp dir build #16859
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| originalPath, | ||
| dirname(__dirname) | ||
| ] | ||
| ig.add(['.gitignore', 'node_modules', 'dist', 'build']); |
There was a problem hiding this comment.
is it safe to not copy node_modules? I'm not familiar yet with the temp dir change (which is why I requested a review from @mydea) so it might be fine, if we install deps after we copied everything to the temp dir.
There was a problem hiding this comment.
this should be fine, we install dependencies inside of the tmp dir then. However, this should not really be necessary after this has settled - if you just run yarn clean:all once, all of these things will not exist in the monorepo anymore anyhow 🤔 this is why I did not add any handling for this, not sure if we want to add this complexity here...?
TLDR this is only a "problem" if you have stale state from previous local runs. Running yarn clean should actually get rid of this fully.
There was a problem hiding this comment.
Yes, you are right, but... Usually , while developing tests, you will run them more than once (locally) and after each run you have to do pnpm install inside test-app dir. That's why this PR, also, removes:
await asyncExec('pnpm clean:test-applications', { env, cwd: __dirname });
It is useless and annoying because test-app is being copied to tmp dir.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
dev-packages/e2e-tests/run.ts
Outdated
| } | ||
| } catch (error) { | ||
| console.error(error); | ||
| process.exit(1); | ||
| } finally { | ||
| await cleanup(); |
There was a problem hiding this comment.
I do not think we want to cleanup when tests fail - the reason being that it makes debugging stuff super hard! Right now, if a test fails you can cd into the tmp dir and debug the actual application state properly. Since all the code is in a tmp dir, it should (in theory) be cleaned up by the OS anyhow after some time 🤔
There was a problem hiding this comment.
No, OS will clean it up after reboot (at least linux). What if we will make it configurable as SKIP_REGISTRY for example?
There was a problem hiding this comment.
alternative idea: What if we prefix the temp dirs we create somehow (e.g. sentry-test-…), and then make sure that running yarn clean also cleans the tmp dirs?
There was a problem hiding this comment.
alternative idea: What if we prefix the temp dirs we create somehow (e.g. sentry-test-…), and then make sure that running
yarn cleanalso cleans the tmp dirs?
Yes, why not.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
0xbad0c0d3
force-pushed
the
fix/tmp-dir-build
branch
from
4c548e8 to
550a491
Compare
Corrected a spelling error in the console log output from "Runnings tests" to "Running tests" for clarity. This change ensures proper messaging during test execution.
Removed the `clean:test-applications` command from e2e test setup as it is unnecessary. Improved the `copyToTemp` function by integrating `.gitignore` and `node_modules` filtering to exclude unwanted files during the copy process. This ensures a cleaner and more efficient temporary workspace.
Updated the `copyToTemp` function to respect ignore patterns defined in `.gitignore` files and exclude unwanted files like `node_modules`, `dist`, and `build`. This ensures that only relevant files are copied to the temporary directory, improving efficiency and alignment with project standards.
Replace manual stdout/stderr piping with predefined stdio settings for concise and reliable output handling. Also, update the asyncExec function signature to use a stricter SpawnOptions type with omitted 'shell' and 'stdio' properties.
Replaced inline `rimraf` calls with a dedicated `clean.ts` script for better maintainability. Standardized temporary directory management by introducing a consistent prefix and leveraging `path.basename` for clarity. These changes improve code organization and simplify future modifications.
0xbad0c0d3
force-pushed
the
fix/tmp-dir-build
branch
from
550a491 to
0420ec5
Compare
| function asyncExec(command: string, options: Omit<SpawnOptions, 'shell'|'stdio'>): Promise<void> { | ||
| return new Promise((resolve, reject) => { | ||
| const process = spawn(command, { ...options, shell: true }); | ||
|
|
||
| process.stdout.on('data', data => { | ||
| console.log(`${data}`); | ||
| }); | ||
|
|
||
| process.stderr.on('data', data => { | ||
| console.error(`${data}`); | ||
| }); | ||
| const process = spawn(command, { ...options, shell: true, stdio: ['ignore', 'inherit', 'inherit'] }); |
There was a problem hiding this comment.
Not necessary, but why not to have it more clean
| const tmpPrefix = join(tmpdir(), 'sentry-e2e-tests-') | ||
| await rimraf(`${tmpPrefix}*`, { glob: true }); |
There was a problem hiding this comment.
This is here, because yarn clean will also delete everything inside test-apps, which is good, but not with active developing. And by doing like this, I believe that we will be able to debut issues, between the executions
Noticed the issue with abandoned "tmp" dirs during local tests development.