Bug 1183516

Summary: yast-bootloader proposal code too generic and not sufficiently auto-tested
Product: [openSUSE] openSUSE Tumbleweed Reporter: Stefan Hundhammer <shundhammer>
Component: YaST2Assignee: YaST Team <yast-internal>
Status: CONFIRMED --- QA Contact: Jiri Srain <jsrain>
Severity: Normal    
Priority: P5 - None CC: jreidinger, mrmazda
Version: Current   
Target Milestone: ---   
Hardware: All   
OS: openSUSE Tumbleweed   
URL: https://trello.com/c/umsAMUg2
Whiteboard:
Found By: --- Services Priority:
Business Priority: Blocker: ---
Marketing QA Status: --- IT Deployment: ---

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