Attention is currently required from: Edward O'Callaghan, Jes Klinke, Nikolai Artemiev, Thomas Heijligen.

Patch set 5:Code-Review +1

View Change

4 comments:

• Patchset:

  • Patch Set #2:

    I have added a few more cases to the test. […]

    Yes, existing test is running `run_basic_lifecycle` which expects the lifecycle to run successfully.

    There is another method in our test framework, which tests initialisation error (invalid value for programmer param for example). This is `run_init_error_path` here https://review.coreboot.org/plugins/gitiles/flashrom/+/refs/heads/main/tests/lifecycle.c#73

    If you would like to add a test to verify that init fails with invalid value of param, you can use run_init_error_path for that. I suspect `raiden_debug_io` would not need that many mocks, maybe even no mocks at all: if init fails early those mocks are never called.

• File doc/classic_cli_manpage.rst:

  • Patch Set #4, Line 873:

    I have attempted to merge your suggested paragraph into the existing documentation.

    Looks really good

• File doc/classic_cli_manpage.rst:

  • Patch Set #5, Line 871:

    trailing whitespace

    Otherwise the text is very good!

• File tests/raiden_debug_spi.c:

  • Patch Set #5, Line 112:

    
    	snprintf(raiden_debug_param, sizeof(raiden_debug_param),
                     "address=%d,target=AP", USB_DEVICE_ADDRESS);
    	run_basic_lifecycle(state, &raiden_debug_io, &programmer_raiden_debug_spi, raiden_debug_param);
    
    	snprintf(raiden_debug_param, sizeof(raiden_debug_param),
                     "address=%d,target=ec", USB_DEVICE_ADDRESS);
    	run_basic_lifecycle(state, &raiden_debug_io, &programmer_raiden_debug_spi, raiden_debug_param);
    
    	snprintf(raiden_debug_param, sizeof(raiden_debug_param),
                     "address=%d,target=0", USB_DEVICE_ADDRESS);
    	run_basic_lifecycle(state, &raiden_debug_io, &programmer_raiden_debug_spi, raiden_debug_param);
    
    	snprintf(raiden_debug_param, sizeof(raiden_debug_param),
                     "address=%d,target=1", USB_DEVICE_ADDRESS);
    	run_basic_lifecycle(state, &raiden_debug_io, &programmer_raiden_debug_spi, raiden_debug_param);
    

    This is great, thank you so much for adding test coverage!

    The only thing: let's make each test case a separate test. Which means, initial test case stays in `raiden_debug_basic_lifecycle_test_success` function, and the other 4 you add into their own test functions.

    I don't mind long names for test functions, so you can name them for example:
    raiden_debug_targetAP_basic_lifecycle_test_success
    raiden_debug_targetEC_basic_lifecycle_test_success
    raiden_debug_target0_basic_lifecycle_test_success
    raiden_debug_target1_basic_lifecycle_test_success

    The definitions of `raiden_debug_fallback_open_state` and `raiden_debug_io` at the beginning of each function you can just copy-paste.

    Then you need to add 4 new functions into:
    tests/tests.c
    tests/tests.h
    and at the bottom of this file add SKIP_TEST macro.

    And that should be it!

To view, visit change 77999. To unsubscribe, or for help writing mail filters, visit settings.