-
Notifications
You must be signed in to change notification settings - Fork 3k
Extends test set for Thread signals #5123
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
41513a4
to
c10f8b2
Compare
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 this an old comment?
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.
What does this comment mean?
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.
X. - indicates subtest number
X.1, X.2, ... - indicates subtest flow
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.
comment added
c10f8b2
to
4797a06
Compare
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.
Could you make sure the tests are small, actual unit tests. And that the description are clear and accurate please.
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.
Can we not have such long tests? I think having to number and subnumber the flows means it's too complicated. Could we redesign it? Maybe split it up in separate test cases and use templates.
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.
Can you rewrite the description to be a bit more grammatically correct please.
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 can clearly see two test cases here (template?).
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'm not sure what that does reading the description.
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 thin that should be rewritten to closer match the test case, something like:
Given two threads A and B
When thread A waits for a signal
and thread B calls @A signal_set
Then thread A wakes up and receives correct signal
@maciejbocianski please look at the latest review comments |
4797a06
to
cbc00dc
Compare
cbc00dc
to
5fd2390
Compare
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.
typo wiat -> wait
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.
Why don't we assert correct status here? But we pass it back to main thread?
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 assumed that asserting in main function will make it easier to analyze the tests
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 it works both ways, with 'where did that value came from' factor. @0xc0170 any preferences from your side?
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.
My preference would be the failure as soon as it happens (in this scope in this case)
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.
can we make the names a bit more readable? eg signal_release_wait_release or something more abstract even
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.
When thread A calls -> and similar for all of the descriptions
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 running
or one running thread
-> and something similar for other descriptions
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.
Could the first line for all test document, on a high level, what the tests asserts? Something like "Validate that set signals accumulate and cause signal wait to return"
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.
Maybe we could also validate that:
- we can set a signal then clear or wait and then set it again
- what happens when we set the same signal twice
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.
wiat -> wait
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.
wiat -> wait
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 don't like 0
in names, can we have some more readable names please
de8f98d
to
1b3b1ce
Compare
@bulislaw Could you review again? |
Build : SUCCESSBuild number : 178 Skipping test trigger, missing label 'NEED CI' |
/morph test |
/morph uvisor-test |
Test : FAILUREBuild number : 124 |
27d73ae
to
37e6eee
Compare
Failing test: |
37e6eee
to
e174bd4
Compare
/morph build |
Build : SUCCESSBuild number : 357 Triggering tests/morph test |
Test : FAILUREBuild number : 170 |
@mprse could you look on lptimer fail in test
|
could we rerun morph ? |
/morph build |
Build : SUCCESSBuild number : 497 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 118 |
/morph test |
Test : SUCCESSBuild number : 316 |
Lets retest /morph build |
Build : SUCCESSBuild number : 545 Triggering tests/morph test |
Test : SUCCESSBuild number : 355 |
Exporter Build : SUCCESSBuild number : 161 |
Description
New test suite for Thread signals
Status
READY