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

use arrays for tests #60

Merged
merged 2 commits into from
Mar 14, 2021
Merged

use arrays for tests #60

merged 2 commits into from
Mar 14, 2021

Conversation

gurgeous
Copy link
Contributor

I moved the tests into arrays so they are all handled in the same way. This is prep for adding a bunch more tests from the top1000. For each icon, I check the url, size and format. I also made it so that the VCR path is inferred from the URL host.

Copy link
Owner

@mat mat left a comment

Choose a reason for hiding this comment

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

👍

{URL: "https://aws.amazon.com/favicon.ico", Width: 16, Height: 16, Format: "ico", Bytes: 1150, Sha1sum: "a64f3616e3671b835f8b208b7339518d8b386a08"},
func TestFetchIcons(t *testing.T) {
tests := []testFetch{
// alibaba - base tag without scheme
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe even worthwhile to treat this as data by adding it as e.g. testName to testFetch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a bad idea. Only reason I didn't was because I was trying to keep that first line a reasonable length. I'll poke around a bit more for the next PR.

Copy link
Owner

Choose a reason for hiding this comment

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

Sure, just a thought, judge for yourself in we Gaia anything from it :–)

@mat mat merged commit 1579f26 into mat:master Mar 14, 2021
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.

2 participants