Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57436 )
Change subject: tests: Extract setup and teardown for chip tests ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
File tests/chip.c:
https://review.coreboot.org/c/flashrom/+/57436/comment/4e305b53_15b3b205 PS1, Line 29: g_chip_state I wonder if it is time to embed this into `data` of the flashctx to avoid possible test coupling. Things like `reset_global_chip_state()` are signs that this could be a good idea to consider.
https://review.coreboot.org/c/flashrom/+/57436/comment/55a52ebf_6b509d87 PS1, Line 89: static void reset_global_chip_state() : { : g_chip_state.unlock_calls = 0; : } Just inline this function to the one call site.
OK more generally in OOP speak here; It is best to avoid having global state management functions which are akin to a singleton class and instead use generators for this kind of state.
https://review.coreboot.org/c/flashrom/+/57436/comment/74a8e006_b2731b70 PS1, Line 94: setup_for_chip
Usually this would be a statement, e.g. just `set_up_chip`.
+1 drop the _for_ just `setup_chip()`
-
Edward O'Callaghan (Code Review)