-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Cleanup file_packager python code #10177
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
Conversation
Also clean up the test code (test change should be NFC).
tests/test_other.py
Outdated
# verify js output file is immutable when metadata is separated | ||
shutil.copy2('immutable.js', 'immutable.js.copy') # copy with timestamp preserved | ||
time.sleep(3.0) |
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.
Please add a comment justifying this, as in general a sleep like that looks very dangerous (but here it is indeed necessary, I agree).
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.
oops I certainly didn't mean to land that ..
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.
No wait, I think it's good? It ensures the timestamp would be different, so it forces the test to actually check, instead of by virtue of being fast enough having the same time.
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.
Is it worth ? Isn't the really important thing that the contents don't change?
Anyway I put it back with a comment.
I think the test failure here is unrelated. Looks related to #10145. OK to land (since I think this is causing failures on the emscripten-releases CI). |
? |
The error does look unrelated to this PR, yes, lgtm. |
Also clean up the test code (test change should be NFC).
Also clean up the test code (test change should be NFC).