-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix wasm2js0 memory growth tests #25000
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
Fix wasm2js0 memory growth tests #25000
Conversation
…test_minimal_runtime_memorygrowth
test/test_core.py
Outdated
self.do_runf(src, '*pre: hello,4.955*\n*hello,4.955*\n*hello,4.955*') | ||
if self.is_wasm2js() and '-O0' in self.cflags: | ||
expect = '*pre: hello,4.955*\nWarning: Enlarging memory arrays, this is not fast! 16908288,20316160\n*hello,4.955*\n*hello,4.955*' | ||
else: |
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.
Perhaps it would be simpler to add a helper function self.remove_growth_warning(output)
which would remove the warning, and call it from these places?
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.
Hmm, the output is handled internally inside self.do_runf(src, expect)
, the test code here doesn't get access to it, but only feeds the expectation. So the function self.do_runf(src, expect)
would have to be broken apart for that.
Or did you mean that the remove_growth_warning()
function would be called on these expectation strings?
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.
Ah, I thought do_runf
had a way to filter strings. We used to, but I guess it was removed ("nicerizer function"). Though there is a regex
param, but using it on these strings could be annoying...
My concern is that the hardcoded 16908288,20316160
figures will get out of date and need annoying updates, so I think it is worth finding a way to avoid that.
One way might be to avoid using do_runf
here, and call self.build
then run and compare manually.
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.
If you want to do the assertion checking outside of do_runf
i think you can just grab the output from the return value:
output = self.do_runf(...)
output = .. filter output ..
self.assertContained(expected, output)
It not has nice a doing it in single do_runf
call, but it should work.
Alternatively we could find a way to split the Warning
output (stderr) from the actual test output (stdout). Perhaps a stdout_only, param or something like 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.
i think you can just
Thanks, super useful. Didn't realize that the test would skip asserting and return output like this. Refactored to use this format, how does this look?
test/test_core.py
Outdated
expect = '*pre: hello,4.955*\n*hello,4.955*\n*hello,4.955*' | ||
output = self.do_runf(src) | ||
output = self.remove_growth_warning(output) | ||
self.assertContained(expect, output) |
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.
nit: Maybe just inline the expect
here and below?
lgtm either way.
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.
ok
Fix wasm2js.test_memorygrowth, wasm2js.test_memorygrowth_2, wasm2js0.test_minimal_runtime_memorygrowth