Skip to content
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

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

jelly
Copy link
Member

@jelly jelly commented Aug 4, 2023

I hope it's not to long 😄

A nicely rendered document can be viewed here

@jelly jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Aug 4, 2023
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
Copy link
Member

@martinpitt martinpitt left a 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.

test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
@martinpitt
Copy link
Member

@jelly Most of my earlier review items are still outstanding. You may have not seen them as they are (unhelpfully) hidden behind an expander?

@jelly
Copy link
Member Author

jelly commented Aug 24, 2023

@jelly Most of my earlier review items are still outstanding. You may have not seen them as they are (unhelpfully) hidden behind an expander?

Sorry, I saw them now I hope I've addressed everything.

Edit: something which is missing is dropping nondestructive everywhere and switching it to "nondestructive" I suppose?

@jelly
Copy link
Member Author

jelly commented Aug 30, 2023

@martinpitt gentle reminder

Copy link
Member

@martinpitt martinpitt left a 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!

test/run Outdated Show resolved Hide resolved
test/run Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Show resolved Hide resolved
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.
Copy link

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:

;)

@jelly jelly force-pushed the test-architecture-doc branch 2 times, most recently from c23130a to e0cda54 Compare September 12, 2023 09:46
Copy link
Member

@martinpitt martinpitt left a 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.

test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
martinpitt
martinpitt previously approved these changes Sep 12, 2023
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers!

Copy link
Member

@subhoghoshX subhoghoshX left a 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 :)

test/ARCHITECTURE.md Show resolved Hide resolved
test/ARCHITECTURE.md Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
test/ARCHITECTURE.md Outdated Show resolved Hide resolved
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers!

@jelly jelly merged commit 9c73bec into cockpit-project:main Sep 13, 2023
33 of 34 checks passed
@jelly jelly deleted the test-architecture-doc branch September 13, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-test For doc/workflow changes, or experiments which don't need a full CI run,
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants