Skip to content

Cleanup file_packager python code#10177

Merged
sbc100 merged 4 commits intoincomingfrom
file_packager_fixes
Jan 9, 2020
Merged

Cleanup file_packager python code#10177
sbc100 merged 4 commits intoincomingfrom
file_packager_fixes

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 9, 2020

Also clean up the test code (test change should be NFC).

Also clean up the test code (test change should be NFC).
@sbc100 sbc100 requested a review from kripken January 9, 2020 18:45
self.assertExists('immutable.js.metadata')
# 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)
Copy link
Member

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).

Copy link
Collaborator Author

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 ..

Copy link
Member

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.

Copy link
Collaborator Author

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.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 9, 2020

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).

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 9, 2020

?

@kripken
Copy link
Member

kripken commented Jan 9, 2020

The error does look unrelated to this PR, yes, lgtm.

@sbc100 sbc100 merged commit 5b2fc36 into incoming Jan 9, 2020
@delete-merged-branch delete-merged-branch bot deleted the file_packager_fixes branch January 9, 2020 23:24
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
Also clean up the test code (test change should be NFC).
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