Skip to content

fix(interop.py): override instead of append to row[server]#375

Merged
marten-seemann merged 1 commit intoquic-interop:masterfrom
mxinden:uninitialized
Mar 26, 2024
Merged

fix(interop.py): override instead of append to row[server]#375
marten-seemann merged 1 commit intoquic-interop:masterfrom
mxinden:uninitialized

Conversation

@mxinden
Copy link
Contributor

@mxinden mxinden commented Mar 25, 2024

Previously when calculating each cell of the measurements result table, instead of always setting a cell, a cell would be appended in case one already existed. Appending a cell to another doesn't make sense in this context. In addition this code also errors, given that on the first cell row[server] += results in a KeyError.

 Traceback (most recent call last):
  File "run.py", line 168, in <module>
    sys.exit(main())
  File "run.py", line 150, in main
    return InteropRunner(
  File "/home/runner/work/quic-go/quic-go/quic-interop-runner/interop.py", line 529, in run
    self._print_results()
  File "/home/runner/work/quic-go/quic-go/quic-interop-runner/interop.py", line 243, in _print_results
    row[server] += "\n".join(results)
KeyError: 'quic-go-latest'

See e.g. https://github.com/quic-go/quic-go/actions/runs/8159188951/job/23046885072.

This commit fixes the above simply by setting the cell, not appending it for the case where one already exists.

Bug introduced in #355

Previously when calculating each cell of the _measurements_ result table,
instead of always setting a cell, a cell would be appended in case one already
existed. Appending a cell to another doesn't make sense in this context. In
addition this code also errors, given that on the first cell `row[server] +=`
results in a `KeyError`.

```
 Traceback (most recent call last):
  File "run.py", line 168, in <module>
    sys.exit(main())
  File "run.py", line 150, in main
    return InteropRunner(
  File "/home/runner/work/quic-go/quic-go/quic-interop-runner/interop.py", line 529, in run
    self._print_results()
  File "/home/runner/work/quic-go/quic-go/quic-interop-runner/interop.py", line 243, in _print_results
    row[server] += "\n".join(results)
KeyError: 'quic-go-latest'
```

See e.g. https://github.com/quic-go/quic-go/actions/runs/8159188951/job/23046885072.

This commit fixes the above simply by setting the cell, not appending it for the
case where one already exists.

Bug introduced in quic-interop#355
@marten-seemann
Copy link
Collaborator

I'm not sure I understand what's going on here. As far as I can see, this was not introduced in #355, was it?

https://github.com/quic-interop/quic-interop-runner/pull/355/files#diff-a1f0ac428938a846f118f59aa8e2e66b90bff7b498b928204eb3bfcb41f1f528L228

@mxinden
Copy link
Contributor Author

mxinden commented Mar 26, 2024

Before #355 it was:

row += ["\n".join(results)]

This adds to the row itself. The row is initialized to row = [client] beforehand, thus += is fine.

After #355 it was:

row[server] += "\n".join(results)

This adds to the cell (i.e. row[server]) within the row. The row is initialized to rows.setdefault(client, {}). But instead of the row, the cell (i.e. row[server]) is accessed. The cell might not be initialized and thus the +=, when retrieving the existing value before adding the new value, will run into a KeyError.

Does that make sense @marten-seemann?

@marten-seemann
Copy link
Collaborator

Thanks for the detailed explanation!

@marten-seemann marten-seemann merged commit be2b3bc into quic-interop:master Mar 26, 2024
@WesleyRosenblum
Copy link
Contributor

Something still seems broken: https://github.com/quic-interop/quic-interop-runner/actions/runs/8526238616/job/23355310162#step:17:31

@mxinden
Copy link
Contributor Author

mxinden commented Apr 3, 2024

Thank you @WesleyRosenblum for spotting this. Should be fixed with #380.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants