Conversation
73cdbbc to
4b8947f
Compare
4070855 to
5161356
Compare
|
The tests looks good to me, any idea why we are getting the The end of the log is this: which could be a timeout, although I would have hoped that I'd put a timeout message in the script if it stopped because of one. The same test does not seem to time out for other platforms and it should be platform independent. Maybe the default 1GHz assumption is not right for qemu? |
No, the old tests don't test anything. You're supposed to enable debug prints and check that the result is what you'd expect. I'm busy improving them and turning them into proper tests. That said, detecting hangs would complicate individual tests which currently are very simple. (In this case, just two Best way to handle timeouts is to have generic test timeout handling in the upper test layer. But considering there isn't even a generic fault handler and how wanting the test infrastructure is, I'm not keen on wasting too much time on this (I'd rather spend my time on rewriting the whole thing personally, to reduce over all complexity). Better to reduce the timeouts in CI for now.
That test doesn't use those. It was caused by the |
|
(Still squashing and style update to do, but I'm happy with the content) |
Signed-off-by: Indan Zupancic <indan@nul.nu>
Signed-off-by: Gerwin Klein <gerwin.klein@proofcraft.systems>
|
Maybe trivial, but I think you meant to say CMake style instead of camke style. Too late to change though. :-) |
|
Indeed I did. Sorry about that. |
| error = seL4_DomainSet_Set(env->domain, CONFIG_NUM_DOMAINS - 1, | ||
| env->tcb); | ||
| /* Reset our domain */ | ||
| seL4_DomainSet_Set(env->domain, 0, env->tcb); |
There was a problem hiding this comment.
This looks weird? Are both of these lines still needed?
There was a problem hiding this comment.
Not sure, can't remember whether it was just undoing any changes the test does, or for a specific reason.
I also don't know whether sel4test creates a new task for each tests or not. If it does, then it's redundant for sure.
There was a problem hiding this comment.
I mean that the added seL4_DomainSet_Set() function call comes directly after another seL4_DomainSet_Set() function call to the same tcb. So the previous call looks like it should be removed?
I think this line should either be removed, or a comment added explaining why the the cleanup needs to set the domain first to CONFIG_NUM_DOMAINS - 1 and then again to 0 for the same thread.
There was a problem hiding this comment.
Is this not an actual test? It is setting the domain of the current thread and then resetting it. The reset might not be needed if the thread is suspended anyway afterwards, but it does look cleaner to me to leave it in the domain it started in.
There was a problem hiding this comment.
Yes, the first line is the actual test, hence we store the result in error.
The second line is the cleanup.
There was a problem hiding this comment.
ohhh. The way that GitHub ux was presenting the change it looked like this was a test cleanup function...
Test with seL4/seL4#1511.