Bug 1183516 - yast-bootloader proposal code too generic and not sufficiently auto-tested
yast-bootloader proposal code too generic and not sufficiently auto-tested
Status: CONFIRMED
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: YaST2
Current
All openSUSE Tumbleweed
: P5 - None : Normal (vote)
: ---
Assigned To: YaST Team
Jiri Srain
https://trello.com/c/umsAMUg2
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-15 09:46 UTC by Stefan Hundhammer
Modified: 2021-03-15 17:09 UTC (History)
2 users (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Hundhammer 2021-03-15 09:46:30 UTC
This is fallout of bug #1183500 which turned out to be a duplicate of bug #1179482:

A new symbol for the YaST bootloader proposal was introduced, but only half-way: There was the hyperlink in the generated HTML, but no function counterpart.

This should not happen. This was a totally avoidable problem.

Worse, it's hard to find if you are not that familiar with the code: A simple "grep" for the symbol that threw an exception does not lead you to the code that actually does something because a symbol like "disable_update_nvram" is taken apart and processed based on string fragments.

Since we keep talking so much about test coverage, why don't we have a test that covers this?

Why don't we have a test that iterates over ALL the proposal fragments and checks if there is a method for each hyperlink so this kind of problem doesn't keep coming back?

Worse, since the strings are copied & pasted again and again, a simple typo will already lead to a crash.

And speaking of which, this kind of problem should definitely not be a showstopper for an installation / upgrade as happened here. This shouldn't happen in the first place because we clearly should have a test that checks it, but if something happens, it should not crash the installation and thus throw away all data and work that the user put into it.
Comment 1 Stefan Hundhammer 2021-03-15 09:51:27 UTC
Relevant code locations:

Generating the HTML:

https://github.com/yast/yast-bootloader/blob/master/src/lib/bootloader/grub2base.rb#L425


The array holding all href targets:

https://github.com/yast/yast-bootloader/blob/master/src/lib/bootloader/proposal_client.rb#L73

(checking them all should be another test)


Taking the clicked link target apart:

https://github.com/yast/yast-bootloader/blob/master/src/lib/bootloader/proposal_client.rb#L105


The code that actually handles the hyperlink:

https://github.com/yast/yast-bootloader/blob/master/src/lib/bootloader/proposal_client.rb#L358-L387