-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
test: add a document describing Cockpit's test Architecture #19177
Conversation
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.
Thanks for documenting this! Initial review round. You also have a lot of runaway sentences there -- this needs some more commas and other interpunction. I added suggestions for the most important cases.
b9f9291
to
474e70b
Compare
@jelly Most of my earlier review items are still outstanding. You may have not seen them as they are (unhelpfully) hidden behind an expander? |
ae29be6
to
74ef124
Compare
Sorry, I saw them now I hope I've addressed everything. Edit: something which is missing is dropping |
@martinpitt gentle reminder |
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.
Sorry for not seeing the update earlier. Next round.
Thanks!
74ef124
to
367dad0
Compare
test/ARCHITECTURE.md
Outdated
The test runner inspects all `running_tests` in a tight loop and polls if the | ||
test is still running. If the process stopped or exited the test output and | ||
`returncode` is saved. Depending on the `returncode` the test runner makes a | ||
decision on what to do next as described in the diagram below. |
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.
Hi Jelle,
I believe this should be rephrased as:
decision on what to do next as described in the [flow chart](https://xkcd.com/518/) below:
;)
c23130a
to
e0cda54
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.
Thanks! I stopped grammar nags, and just have a few remaining correctness issues.
036094c
to
5da0738
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.
Cheers!
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.
It was a great read :)
c2145c1
to
f0976b3
Compare
f0976b3
to
0ec5feb
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.
Cheers!
I hope it's not to long 😄
A nicely rendered document can be viewed here