Tidy up test.sh and make it clean up properly. #1074
Merged
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
vroulet
reviewed
Sep 27, 2024
There was a problem hiding this comment.
Thanks for looking at that!
Tried on my end, works well. Tested (i) without error, (ii) with error induced manually to check if everything cleaned up well, (iii) with rerunning after failing test.
Just sync with head to avoid the doctest error.
Finally, why are you saying that this PR only partially solves the issues of #1067?
carlosgmartin
force-pushed
the
tidy_test_sh
branch
from
b0cd5a6
to
e3b5194
Compare
Sorry for the unclear wording. I just meant that #1067 was also addressed by #1068 and #1071. On my machine, Does this happen on your machine? How would you prefer to address it? |
carlosgmartin
force-pushed
the
tidy_test_sh
branch
from
e3b5194
to
2b639ba
Compare
|
Yes, I had seen it as I was doing the review. If you have a better solution, I'll take it but I think it's ok to simply clean it up at the end. Thank you again for taking care of this. We are used to run tests on google's side and it will really help users to be able to actually run tests locally properly. |
|
@vroulet We could also add it to .gitignore. Might be useful to keep the file for debugging/inspection purposes. What do you think? |
|
Come to think of it, the same might be true for the virtual env created and used by the test script (keeping it in a gitignored folder, e.g. |
That's the right solution. I don't think we will ever need these logs but I'd prefer to keep test.sh as clean as possible without weird exceptions like this one.
To be honest I don't know why we had this clean up logic in the first place (I'm not one of the original authors of the package). But I don't think either of us are eager to understand why if we start using a gitignore and finding bugs. Your PR is quite clean and I think it solves well the issue you raised so just add |
carlosgmartin
force-pushed
the
tidy_test_sh
branch
from
2b639ba
to
3fef47e
Compare
|
@vroulet Done. |
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.
Partially addresses #1067. Currently,
test.shleaves a wheel fileoptax-0.2.4.dev0-py3-none-any.whlin the root directory of the repo. It also doesn't run cleanup when an intermediate failure occurs.