Skip to content

Conversation

@Hook25
Copy link
Collaborator

@Hook25 Hook25 commented Jun 8, 2023

Description

This fixes the text in the interrupt menu to be easier to understand, also removes the outdated nomenclature for remote and service

Resolved issues

Fixes: #444, #129, #19, #550

Documentation

N/A

Tests

N/A

Minor: Fix a small edge case in test kill
@Hook25 Hook25 requested a review from yphus June 8, 2023 10:41
Copy link
Collaborator

@pieqq pieqq 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 cleaning up the wording in this screen! @bladernr will be very happy about that :D

A few comments below.

Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

Added 3 cents in response to Pierre's comments.

@pieqq
Copy link
Collaborator

pieqq commented Jun 13, 2023

FYI, I'm working on rewording the docs (https://warthogs.atlassian.net/browse/CHECKBOX-658) so I'll rework the section that explains the screen you are modifying based on whatever will come out from this PR!

Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

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

A few more comments. I know it's just words and not code, but words are somehow even more important because they directly face the users 😄

_("Stop the test in progress and move on to the next"),
_("Disconnect but let the test session continue (CTRL+C)"),
_("Exit and stop the Checkbox service on the testbed at {}".format(host)),
_("Finalize this session and launch a new test plan"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Finalize" may sound like the session will be wrapped up, and the user will have the opportunity to upload it to C3 or to download it. It's not the case: Checkbox removes the incomplete flag from the session metadata flags section, marking it as "finished" even though nothing has been done with it. By doing so, this session will not be resumable, but since there are probably jobs that have not been run, submitting it to C3 would not be very useful.

So I guess "Abandon" is more accurate.

Moreover, since we are talking about a session, it's better to stick to this word for the second part of the sentence, so you could write "and start a new one" for instance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just one caveat to Abandon, the session is still there and downloadable for inspection, I wanted a verb to signal the fact that we are leaving it as finished and not removing it

Copy link
Collaborator

Choose a reason for hiding this comment

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

"Stop the test in progress and move on to the next"
One minor comment, I'd be just bit more specific here and say "test case" rather than "test" as just "test" feels too generic, is it the test case, the whole test suite? a group of tests (e.g. the CPU tests)? But maybe that's just how I was reading it this morning.

"Finalize this session and launch a new test plan"
And with this one... what does "finalize" mean? That wording always felt awkward to me...

"End this test session, preserving its data, and launch a new test session" sounds more accurate to me, but that's just IMO.

I feel like these sorts of discussions always feel a bit like bike shedding, despite the fact that getting the words right is so very important, as @pieqq pointed out.

Copy link
Collaborator

@pieqq pieqq 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 this change, and thanks for your patience with all the back and forth renaming strings implies :)

+1

@pieqq pieqq merged commit b1ac39c into main Jun 14, 2023
@pieqq pieqq deleted the fix_interrupt_menu_text branch June 14, 2023 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interrupt menu uses outdated master/slave nomenclature

5 participants