Use JSON snapshots for all code size tests. NFC #24672
Conversation
RReverser
changed the title
There was a problem hiding this comment.
Thanks for doing this! I've been thinking about doing this myself for a while now.
I really like the idea, especially for the code sizes themselves. I'm less sure about the symbol lists since the extra syntax of quotes and commas makes them harder to read than flat files.
Would you consider using json only for the file sizes and continue to use flat files for symbol lists? Or maybe its not worth it?
I could, but I find it kind of nice to have in the same context for the same reason it's nice to have different sizes in the same context - eg there were times when code size increased because some symbol was accidentally exported that shouldn't have been, and then it's helpful for debugging the increase. I guess we could output YAML / TOML / custom text output if the quotes and commas are the concern? |
No this seems fine to me if its useful to have it all together. |
test/common.py
Outdated
| @@ -1694,14 +1697,76 @@ def assertBinaryEqual(self, file1, file2): | |||
| self.assertEqual(read_binary(file1), | |||
| read_binary(file2)) | |||
|
|
|||
| def check_expected_size_in_file(self, desc, filename, size): | |||
| def check_output_sizes(self, outputs: List[str], metadata=None): | |||
| test_name = self.id().split('.')[-1] | |||
There was a problem hiding this comment.
I guess this makes a lot of sense to set the filename automatically like this, but it would break if we ever had two different test suites with the same test name... i guess that is very unlikely and we would notice right away.
There was a problem hiding this comment.
FWIW that's how existing logic already worked in run_codesize_test, I just copied it here.
|
(I'll do final rebaseline to fix CI before merging) |
RReverser
force-pushed
the
code-size-json
branch
2 times, most recently
from
09926be to
0bb40ba
Compare
|
|
RReverser
force-pushed
the
code-size-json
branch
2 times, most recently
from
43cda7c to
424ba38
Compare
|
@sbc100 I think the CI check expectations script doesn't correctly handle removed/added tests, as it tries to find the same test both before and after this PR and fails. What is the solution in such cases? Manual merge? |
Fair enough we can bypass that check for this PR. |
Can you please do it? I don't have the bypass rights :( |
RReverser
force-pushed
the
code-size-json
branch
from
14759a3 to
c8c225d
Compare
|
I rebaselined & fixed conflicts again, but it might need yet another rebaseline right before you merge it. |
Right now, code size tests in the codebase use two different formats - either everything in a single JSON file, or multiple single-line files.
I found the discrepancy a bit annoying and wanted to unify them.
Between the two, the one with individual files tends to be harder to review as Github (as well as local git tools) add quite a lot of boilerplate UI around each file, you need to manually scan filenames to understand which size diffs are related to each other, and overall feels more difficult to scan quickly as a human.
In this PR I'm unifying both towards a single JSON file format, and other supplementary metadata (
imports,sentetc) goes into the same JSON as well so you can see all relevant context together during reviews.