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
test: Update --tmpdir doc string to say directory must not exist #29498
Conversation
The error message given if passing an existing dir to --tmpdir is confusing so this makes it clear that the directory must not already exist
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
Might you be able to modify the message as well? It will be the most available info when people encounter it. |
lgtm ACK d4e36ae |
Updated in be027e0 I added a line to enable logging and it now gives this error which it did not before because it would throw an error because logging was not enabled New Error message
Previous Error message
|
I may want to find a way to remove the logs after |
force pushed once more eb28434 the error log now makes sense
|
d782db0
to
191b985
Compare
be027e0
to
eb28434
Compare
new error log in c5eb468 let me know if you want to change the verbiage and if the dir exists we use the default location and then raise and exception
|
c5eb468
to
09067cc
Compare
except Exception: | ||
self.options.tmpdir = tempfile.mkdtemp(prefix=TMPDIR_PREFIX) | ||
self._start_logging() | ||
self.log.error("The tmpdir must not already exist {}".format(tmpdirpath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about using a different tempdir than the one the user asked for as fallback to start logging.
I don't think code needs to be written for users that fail to read the docs, and also fail to read the python stacktrace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup that makes sense and I agree reverting back to d4e36ae
09067cc
to
d4e36ae
Compare
rfm? |
ACK d4e36ae |
The error message given if passing an existing dir to --tmpdir is confusing so this makes it clear that the directory must not already exist
This change is motivated by this comment #29335 (comment)