tests/malloc: reduce default chunk size #12813
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
This allows to automatically run the test on very constrained platforms such as arduino-uno (2KB RAM)
aabadie
added
Type: bug
benpicco
approved these changes
Nov 26, 2019
aabadie
changed the title
|
Please activate the |
|
Maybe we should upgrade the RPIs used by Murdock to more powerful ones (RPI3B+ or RPI4). This test works like a charm when run locally on my (powerful ;)) machine (and I also I tested this before this PR was merged of course). Here are the results of a fresh run: GCCLLVMMy guess is that it should be fixed once #12461 is merged. |
|
I think the pi cannot handle the output speed. Even with the larger chunk size it seems like it is failing. |
|
Lowering the With that in mind, not getting this PR in would have prevented the email. But the issue would still be there, only not triggered during CI. And IMHO the emails have the goal to inform about issues. Not merging something that triggers, but doesn't cause the issue, would IMO be in contrast to the goal of the emails. (If someone doesn't what the emails, we could maybe add a separate email list for that. Or receivers just set up filters. But if someone is interested in emails about test failures from nightlies, they should get as many correct bug reports as possible, IMO.) |
Not sure this is the best solution, but we could add a small delay between each allocation/free or between chunk of allocations/free. |
|
I'm not saying the PR shouldn't have been merged. Of course a fixed, but failing, test is better than a false positive test. However, I think a failing nightly test (after @kaspar030 just fixed another failing test) could have been prevented, if better care would have been taken by running the test on the CI before merging. |
|
I think it's more important to take this problem as an opportunity to discuss and agree on a potential fix here (please comment #12813 (comment)) than to trying to blame people because they omitted to enable |
|
I think that @miri64 didn't what to blame anyone, but suggest a way to handle it differently next time. I agree 100% with you that we certainly should be careful to not starting a blame game after nightly failures. Instead, being happy that the bug hunting got much easier by having that info is better :-) (But again, I don't think that's what @miri64 did. To me, it was just a suggestion to take into account for the next similar situation.) |
|
Exactly. I'm sorry if my comment was mistook as pointing the finger. I just wanted to help improve things in the future (and I believe it is already worked upon, that tests always are run before merge unless deactivated - so the opposite of the current situation). |
We should try that, yes. |
|
I'm not sure the delay would help. When sending the test manually to murdock (with a local compile sent to murdock), it succeeds. |
|
The regexps could be the problem, similar to #12128 . The first regexp: I'll try a fix... |
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.
Contribution description
This PR reduces the default value of CHUNK_SIZE from 1024 to 512. This allows to automatically run the test on very constrained platforms such as arduino-uno (2KB RAM).
Testing procedure
Run:
On master it fails because to allocations are performed and the automatic test script checks that at least one allocation is made.
With this PR, there's one allocation performed by the test application so the automatic test script works.
Issues/PRs references
None