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

Clean lint from #1275 #907

Merged
merged 1 commit into from
Mar 27, 2017
Merged

Clean lint from #1275 #907

merged 1 commit into from
Mar 27, 2017

Conversation

paulmelnikow
Copy link
Member

Ah, it makes sense that the lint test will only run on branches that have incorporated the commits which pulled them in. 😄

So the in-PR test results will be more reliable for new PRs than for old ones.

@paulmelnikow paulmelnikow requested a review from espadrine March 27, 2017 21:21
@paulmelnikow paulmelnikow added the developer-experience Dev tooling, test framework, and CI label Mar 27, 2017
@espadrine
Copy link
Member

Yes, better rerun Travis builds.

Also, make sure you try PRs (node server 1111 and open http://localhost:1111/try.html) and run npm test locally before merging.

@espadrine espadrine merged commit e8988a9 into badges:master Mar 27, 2017
@paulmelnikow
Copy link
Member Author

Will do.

The idea would be to hit all the .svg endpoints that try.html loads.

What do you check? No 404's and no errors on the server?

@espadrine
Copy link
Member

Depends on the PR. For those that add a badge, I verify that I see the badge, and that all advertised parameters work as intended. Since those PRs should add an example badge to try.html (if they don't, ask that they do), it's usually as simple as filtering to find the right badge.

For other types of patches, I typically verify that they don't break what they modify, and that they don't break the website.

For patches where I have performance concerns, I do performance analysis. The costly parts are text width measurement, heavy downloads and/or parsing, and graphics conversion.

@paulmelnikow paulmelnikow deleted the clean_lint branch March 28, 2017 00:02
@paulmelnikow
Copy link
Member Author

What would you say classifies as a heavy download? In #888, for example, the code loads results for 100 builds at a time, paginating through as it looks for the right one. Each page is roughly 20 kb.

@espadrine
Copy link
Member

espadrine commented Mar 29, 2017

Yeah we don't want to do that. In general, anything that isn't bounded in time or complexity, or that takes always more than 600 ms to load is not good. Also, anything that clogs up the file system… I have this old pain from the bower badge: it downloads code with a git process. I end up having this running in cron on all servers: https://github.com/badges/ServerScript/blob/master/remove-bower-cache.sh

@paulmelnikow
Copy link
Member Author

Re #888: Makes sense. I imagine 99% of projects will get the latest build in the first request. Maybe that would be acceptable. It would be nice to find a way to accomplish the dockerhub build status.

Re bower: Ugh, that is really messy. It must be slow and bandwidth-intensive too.

@paulmelnikow
Copy link
Member Author

Possible alternative: #919

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants