Fix for data directory not being cleared during browser.close() panic #991
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While testing for another change, it became apparent that the panic when working with browser.close() would prevent any of the remaining data directories from being cleaned up in the temporary directory. Instead of panicking, we're going to log an error and carry on with rest of the process to try and close the subprocess and eventually delete the data directory. This change does indeed bring about the behaviour we want even when the context has been closed.
ankur22
force-pushed
the
fix/log-clean-dir
branch
from
f88a8c8
to
4129498
Compare
There will be two temp clean dir tests. We need them to not look at what each other are creating and cause the test to pass/fail. So the current test will need to write to a new dir that is different to what the new test will write to.
When the context is closed before we call browser.close() used to cause a panic on the cdp command to close the browser. Since we've changed this to log instead of panic, we just need to make sure that the data dir does indeed get removed when the cdp command fails.
ankur22
force-pushed
the
fix/log-clean-dir
branch
from
49ee8fd
to
b5ceb5a
Compare
inancgumus
reviewed
Aug 3, 2023
1. Work with MkdirTemp to avoid dir name collisions. 2. Defer the cleanup before checking the error on MkdirTemp. 3. Work with RemoveAll to cleanup sub dirs and files incase the test fails so that the cleanup still occurs as expected. Resolves: #991 (comment) Resolves: #991 (comment)
ankur22
added a commit
that referenced
this pull request
Aug 3, 2023
1. Work with MkdirTemp to avoid dir name collisions. 2. Defer the cleanup before checking the error on MkdirTemp. 3. Work with RemoveAll to cleanup sub dirs and files incase the test fails so that the cleanup still occurs as expected. Resolves: #991 (comment) Resolves: #991 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
While testing for another change, it became apparent that the panic when working with browser.close() would prevent any of the remaining data directories from being cleaned up in the temporary directory. Instead of panicking, we're going to log an error and carry on with rest of the process to try and close the subprocess and eventually delete the data directory. This change does indeed bring about the behaviour we want even when the context has been closed.
Checklist