Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Size Comparison on Pull Requests #2513

Merged
merged 9 commits into from
Mar 18, 2022
Merged

Conversation

futursolo
Copy link
Member

@futursolo futursolo commented Mar 12, 2022

Description

Fixes #2460

This pull request adds a comment on pull requests that compares the size of examples of master and pull requests.

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

Preview

For every pull request, a comment like the following will be made on the pull request automatically.

Size Comparison

examples master (KB) pull request (KB) diff
boids 420.108 413.815 -6.293
contexts 311.164 312.313 +1.149
counter 239.493 239.589 +0.096
dyn_create_destroy_apps 250.018 251.504 +1.486
file_upload 271.926 270.394 -1.532
function_memory_game 458.660 452.327 -6.333
function_router 657.030 4876.129 +4219.099
function_todomvc 445.075 1937.725 +1492.649
futures 485.006 484.910 -0.096
game_of_life 294.672 293.459 -1.213
inner_html 226.233 226.937 +0.703
js_callback 245.771 243.983 -1.788
keyed_list 440.328 437.477 -2.852
mount_point 238.485 235.455 -3.030
nested_list 325.065 326.522 +1.457
node_refs 248.589 249.030 +0.441
password_strength 2207.762 2173.107 -34.654
portals 1405.800 258.956 -1146.844
router 829.770 4074.907 +3245.138
simple_ssr 3462.738 3462.738 0
ssr_router 5439.181 5439.181 0
suspense 305.204 300.256 -4.948
timer 247.290 248.418 +1.128
todomvc 371.790 368.855 -2.935
two_apps 239.160 240.350 +1.189
webgl 243.624 245.023 +1.399

@futursolo
Copy link
Member Author

Hmm, seems like the build size comment is not going to take effect until this pull request is merged.

Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not merge post-size-cmp.yml and size-cmp.yml? It's not like those 2 can run in parallel. It would help keep everything in once place and would also allow us to avoid the upload/download info step.

@futursolo
Copy link
Member Author

futursolo commented Mar 13, 2022

When a workflow is triggered on a pull request, it has no access to post comments on the base repository.

You can use pull_request_target trigger which has access to base repository, but it should not be used to run code inside a pull request.

This means that the code needs to be compiled within the fork and the comment needs to be posted from base repository so we need 2 workflows.

See: https://github.com/yewstack/yew/runs/5520333803?check_suite_focus=true

Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know that GITHUB_SECRET didn't have permissions to post a comment on the pull request.

@hamza1311
Copy link
Member

Why didn't it approve auto approve for you?

Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. If we want to refine this in the future, maybe have a discussion how we want to organize supplementary code, such as the ones in the yaml files.

Comment on lines +28 to +70
from typing import Dict, List, Optional

import os
import json

with open("size-cmp-info/.SIZE_CMP_INFO") as f:
content = json.loads(f.read())

joined_sizes = content["sizes"]

lines: List[str] = []

lines.append("### Size Comparison")
lines.append("")
lines.append("| examples | master (KB) | pull request (KB) | diff |")
lines.append("| --- | --- | --- | --- | ")

for (i, sizes) in joined_sizes:
(master_size, pr_size) = sizes
diff: Optional[int] = None

if master_size is not None and pr_size is not None:
diff = pr_size - master_size

master_size_str = "N/A" if master_size is None else (
f"{master_size / 1024:.3f}" if master_size != 0 else "0"
)
pr_size_str = "N/A" if pr_size is None else (
f"{pr_size / 1024:.3f}" if pr_size != 0 else "0"
)
diff_str = "N/A" if diff is None else (
f"{diff / 1024:.3f}" if diff < 0 else (
f"+{diff / 1024:.3f}" if diff > 0 else "0"
)
)
lines.append(f"| {i} | {master_size_str} | {pr_size_str} | {diff_str} |")

output = "\n".join(lines)

with open(os.environ["GITHUB_ENV"], "a+") as f:
f.write(f"YEW_EXAMPLE_SIZES={json.dumps(output)}\n")
f.write(f"PR_NUMBER={content["issue_number"]}\n")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather have these parts in proper files than inline in yaml files, but it's fine for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could go in the ci directory

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@futursolo, do you want to make this change? If not, please go ahead and merge this

@futursolo futursolo merged commit 605f891 into yewstack:master Mar 18, 2022
@cecton
Copy link
Member

cecton commented Mar 18, 2022

Oh wow that is cool!!!

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.

Adding test to ensure the wasm binaries size don't get too large
4 participants