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

SVG Tester fails to catch some breaking changes #3947

Open
danyx23 opened this issue Sep 10, 2024 · 2 comments
Open

SVG Tester fails to catch some breaking changes #3947

danyx23 opened this issue Sep 10, 2024 · 2 comments

Comments

@danyx23
Copy link
Contributor

danyx23 commented Sep 10, 2024

When we merged #3792 this introduced a bug where an obscure feature for scatter plots that was previously never in fact enabled was applied always. This lead to a few charts showing visual differences. Since this PR came from an external contributor, the SVG tester wasn't running (the PR was not a branch in our repo but in their fork as is the normal flow with third party PRs). This part we will have to deal with by "adopting" a PR and merging it into a branch in our repo first so that all tests can run and then merge that one into master.

This issue is about what happened on the branch that fixed this issue, #3942. This PR removed the offending if branch that was now wrong and thus it changed the charts again - but the SVG tester didn't show any differences! One chart in question (https://ourworldindata.org/grapher/hardware-and-energy-cost-to-train-notable-ai-systems) was a few months old already and Sophia recently updated the svg tester set so it's unlikely that the chart was just not included in the svg test set.

Needs investigation into what went wrong and how to make sure the SVG tester always catches differences like this.

@toni-sharpe
Copy link
Contributor

Speaking ideally: and more broadly, as a 3rd party contributor there shouldn't be anything in an issue or related assets that I'm not able to access. Auth shouldn't block me from independently fixing an issue. That auth may need to be earned via trust, but still attainable.

I know you folks are fairly new the operation of an open source project, well, this is the first time I've got anywhere with one, so we're learning the process on both sides. My opinion: I feel like my work should run green if it's good from a test perspective. Psychology matters again here, but it's also extra work for you having to fiddle with 3rd party PRs before merging them. Again ideally, really you should just be reviewing and merging.

What stops the SVG tester running for external contributors?

@marcelgerber
Copy link
Member

What stops the SVG tester running for external contributors?

It is mostly security reasons, and GitHubs sensible "outside collaborators" defaults for Github Actions.

The risk here is that an outside collaborator can change any code they want, which would then in turn be run on our own server. And, for obvious reasons, we don't want to run untrusted code on our servers.
There are ways in the GitHub UI to allow certain Actions to be run for an outside PR. However, I don't think these are available for the SVG tester - might be to do with repo secrets access for that one? Not sure.

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

No branches or pull requests

3 participants