Changes between Version 1 and Version 2 of org/meetings/2018MexicoCity/Notes/TechnicalDebt


Ignore:
Timestamp:
Oct 4, 2018, 11:50:45 PM (7 months ago)
Author:
nickm
Comment:

--

Legend:

Unmodified
Added
Removed
Modified
  • org/meetings/2018MexicoCity/Notes/TechnicalDebt

    v1 v2  
    11
    2 Net team Technical debt session 10am Oct 3.
    3 ===========================================
     2= Net team Technical debt session 10am Oct 3.
     3
     4== Notes, v1.
    45
    56what's frustrating:
     
    1819  * design for test, design for debuggability
    1920
    20   *
     21`  *
    2122
    2223Considerations:
     
    8889  * Re-design extend_info_t so the reachability checks are done at a more
    8990    logical point.
     91
     92== Notes, v2
     93
     94
     95
     96Testability
     97- amount of stuff you need to put together- would need a full circuit, a lot of complete set up for test state in order to test cells
     98- lack of test functions - not good abstractions- slowely adding these but maybe not enough?
     99
     100- solutions for new code versus solutions for existing code
     101- new code is easier to design for testing/debuggability
     102
     103- tests are not carefully structured to invoke only the code that it is testing
     104- example: function that checks something about a router - router initialization would be invoked, but it isn't actually thoroughly being tested
     105
     106- working out what the test failure is- not easy to tell what the specific issue is that would cause the test failure. Would executing tests in order help with this?
     107
     108- optimizing tests- reducing blocking operations
     109- DNS queries block
     110
     111How to make progress here over the next 6 months?
     112- success with big code movement
     113- introducing new requirements for merging
     114- should make the requirement that all new code has tests- checklists for review?
     115
     116Code review checklist:
     117
     118    1. is there a visible changes file?
     119
     120    2. if this is a new feature, are there unit tests?
     121
     122    3. identify and review upward includes (and whether this functionality should belong in another layer)
     123
     124    - downward layers shouldn't called into upward layers
     125
     126    - core or features layers aren't enforced (layers aren't actually enforced here)
     127
     128    - no new upward includes requirement? we can't actually do this right now, as we have to include config.c
     129
     130    - we should feel free to move code around in the layers right now
     131
     132    - should we have something that counts dependency violations?
     133
     134    4. Look at the _quality_ of the tests. Are they actually meaningful?
     135
     136    5. Testing extreme inputs as well as the normal expected inputs- also should test error cases
     137
     138
     139Improve test vectors?
     140
     141    - test that the function is doing what it is meant to do
     142
     143    - but also having arbitrary values is ok? (unclear)
     144
     145
     146Should we have a new metric for test coverage?
     147
     148    - only code that is directly tested (as opposed to indirectly exercised)
     149
     150    - A static call graph might help with this?
     151
     152
     153Automated PR checks:
     154    - layer violations
     155    - Ram usage
     156   
     157How to make tests more writable?
     158
     159    - run in a bootstrapped tor- bootstrapped small chutney network and then run tests? - this is more like an integration test
     160
     161
     162Should we do any work to clean up existing tests?
     163
     164    - options tests (don't test what they are meant to be testing)
     165
     166    - Tor TLS tests were overfitted
     167
     168    - Config tests are overfitted as well? (expect configs to be parsed in an order) - when we rewrite confing, we should rewrite tests
     169
     170
     171Should we remove bogus tests? We should minimize bogus tests to not pretend to test what they are actually testing (minimize their scope to only test what they actually test)
     172
     173
     174git blame --follow --moved - looking at code that has moved as opposed to new code
     175git --color-moved will make your life much better (should set as default in git config)
     176
     177What is everyone's top "this should be simplified" wishes?
     178
     179    - delete all IPv6 dirports
     180
     181    - go for the biggest c files
     182
     183    - config.c (has bad tests) - subsystem specific configurations
     184
     185    - control.c (well tested by stem)
     186
     187    - crypto state and network code - abstract these so that we can move to multithreading
     188
     189    - things should be able to register for events (publish/subsc
     190
     191
     192
     193    Testability
     194
     195    - amount of stuff you need to put together- would need a full circuit, a lot of complete set up for test state in order to test cells
     196
     197    - lack of test functions - not good abstractions- slowely adding these but maybe not enough?
     198
     199
     200    - solutions for new code versus solutions for existing code
     201
     202    - new code is easier to design for testing/debuggability
     203
     204
     205    - tests are not carefully structured to invoke only the code that it is testing
     206
     207    - example: function that checks something about a router - router initialization would be invoked, but it isn't actually thoroughly being tested
     208
     209
     210    - working out what the test failure is- not easy to tell what the specific issue is that would cause the test failure. Would executing tests in order help with this?
     211
     212
     213    - optimizing tests- reducing blocking operations
     214
     215    - DNS queries block
     216
     217
     218    How to make progress here over the next 6 months?
     219
     220    - success with big code movement
     221
     222    - introducing new requirements for merging
     223
     224    - should make the requirement that all new code has tests- checklists for review?
     225
     226
     227    Code review checklist:
     228
     229    1. is there a visible changes file?
     230
     231    2. if this is a new feature, are there unit tests?
     232
     233    3. identify and review upward includes (and whether this functionality should belong in another layer)
     234
     235    - downward layers shouldn't called into upward layers
     236
     237    - core or features layers aren't enforced (layers aren't actually enforced here)
     238
     239    - no new upward includes requirement? we can't actually do this right now, as we have to include config.c
     240
     241    - we should feel free to move code around in the layers right now
     242
     243    - should we have something that counts dependency violations?
     244
     245    4. Look at the _quality_ of the tests. Are they actually meaningful?
     246
     247    5. Testing extreme inputs as well as the normal expected inputs- also should test error cases
     248
     249
     250    Improve test vectors?
     251
     252    - test that the function is doing what it is meant to do
     253
     254    - but also having arbitrary values is ok? (unclear)
     255
     256
     257    Should we have a new metric for test coverage?
     258
     259    - only code that is directly tested (as opposed to indirectly exercised)
     260
     261    - A static call graph might help with this?
     262
     263
     264    Automated PR checks:
     265
     266        - layer violations
     267
     268        - Ram usage
     269
     270       
     271
     272    How to make tests more writable?
     273
     274    - run in a bootstrapped tor- bootstrapped small chutney network and then run tests? - this is more like an integration test
     275
     276
     277    Should we do any work to clean up existing tests?
     278
     279    - options tests (don't test what they are meant to be testing)
     280
     281    - Tor TLS tests were overfitted
     282
     283    - Config tests are overfitted as well? (expect configs to be parsed in an order) - when we rewrite confing, we should rewrite tests
     284
     285
     286    Should we remove bogus tests? We should minimize bogus tests to not pretend to test what they are actually testing (minimize their scope to only test what they actually test)
     287
     288
     289    git blame --follow --moved - looking at code that has moved as opposed to new code
     290
     291    git --color-moved will make your life much better (should set as default in git config)
     292
     293
     294    What is everyone's top "this should be simplified" wishes?
     295
     296    - delete all IPv6 dirports
     297
     298    - go for the biggest c files
     299
     300    - config.c (has bad tests) - subsystem specific configurations
     301
     302    - control.c (well tested by stem)
     303
     304    - crypto state and network code - abstract these so that we can move to multithreading
     305
     306    - things should be able to register for events (publish/subscribe) - good abstractions should help with this (what would the abstraction for an inter-module communication be?)
     307
     308    - Make C/Rust dependencies automatically checked: https://trac.torproject.org/projects/tor/ticket/24249
     309
     310
     311    TODO:
     312
     313    - make progress by deleting code and deprecating old options