Skip to content

Remove cleanup code from stdlib tests. NFC #23003

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.

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

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Nov 25, 2024

When running tests in emscripten there is no requirement to clean up after yourself. The test framework automatically gives each test a clean directory to work in.

In fact, cleaning up after yourself like this makes debugging harder because in the failure case you can no longer see the state of the filesystem at the point of failure.

In addition, it meant that all these tests were also relying on and testing atexit and signal handling, making the tests more complex that less precise.

As part of this change I also made sure all these tests could be compiled outside of emscripten (on my desktop linux machine).

@sbc100 sbc100 requested a review from kripken November 25, 2024 18:32
@sbc100 sbc100 changed the title Remove cleanup code stdlib tests. NFC Remove cleanup code from stdlib tests. NFC Nov 25, 2024
When running tests in emscripten there is no requirement to clean up
after yourself.  The test framework automatically gives each test
a clean directory to work in.

In fact, cleaning up after yourself like this makes debugging harder
because in the failure case you can not longer see the state of the
filesystem when test fails.

In addition, it meant that all these tests were also relying on and
testing `atexit` and `signal` handling, making the tests more complex
that less precise.

As part of this change I also make sure all these tests could be
compiled outside of emscripten (on my desktop linux machine).
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

There is one exception to this, the IndexedDB and other persistent storage tests in the browser. But looks like you intentionally left those unchanged, very good.

@sbc100 sbc100 enabled auto-merge (squash) November 25, 2024 19:19
@sbc100 sbc100 disabled auto-merge November 25, 2024 19:49
@sbc100 sbc100 merged commit bc2123f into emscripten-core:main Nov 25, 2024
27 of 28 checks passed
@sbc100 sbc100 deleted the remove_cleanup branch November 25, 2024 19:50
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 5, 2024
- Convert to C.
- Move to `test/stdio`

This test was originally added in c008b11 and is not specific to C++
cstdio and was the only file in this directory.

Also remove the cleanup code.  See emscripten-core#23003.
@sbc100 sbc100 mentioned this pull request Dec 5, 2024
sbc100 added a commit that referenced this pull request Dec 5, 2024
- Convert to C.
- Move to `test/stdio`

This test was originally added in c008b11 and is not specific to C++
cstdio and was the only file in this directory.

Also remove the cleanup code.  See #23003.
hedwigz pushed a commit to hedwigz/emscripten that referenced this pull request Dec 18, 2024
- Convert to C.
- Move to `test/stdio`

This test was originally added in c008b11 and is not specific to C++
cstdio and was the only file in this directory.

Also remove the cleanup code.  See emscripten-core#23003.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants