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

Add safe navigation for user_interaction_count #226

Merged
merged 7 commits into from
Oct 27, 2023

Conversation

Spone
Copy link
Contributor

@Spone Spone commented Oct 26, 2023

Close #225

@Spone
Copy link
Contributor Author

Spone commented Oct 26, 2023

Not sure why I have a bunch of new VCR cassettes 🤔

@markets
Copy link
Collaborator

markets commented Oct 27, 2023

Hey @Spone 👋🏼

Yeah! The specs for this project are a bit hard to maintain. For example, for YouTube and Vimeo, we have 2 "versions" for each service: scraper or API.

I've fixed some stuff, mainly related to specs+VCR, here 👉🏼 #227. Could you please update your branch?

To fully run the specs locally, you need a couple of ENV vars in your machine (the ones related to Youtube and Vimeo apis). You can create those keys by yourself (docs), but I can also share with you the ones we are using for dev/testing (ping me via email at [email protected] and I'll send you the keys). The CI already uses them.

@Spone
Copy link
Contributor Author

Spone commented Oct 27, 2023

Thanks @markets! I emailed you for the keys. I updated my branch. Do I need to delete all cassettes that have been created in my PR?

@markets
Copy link
Collaborator

markets commented Oct 27, 2023

Sorry, I'm not really a VCR hardcore user 😅... I'm just using it here because the project was implemented this way when I started contributing. I think new cassettes are created when you add a new spec hitting a new URL, but to be sure, you can delete the new ones and run the specs again (if they appear again, this is probably the expected behavior).

@Spone
Copy link
Contributor Author

Spone commented Oct 27, 2023

I'm hitting 429 Too Many Requests responses when running the tests. I'll wait for a bit and retry.

@Spone
Copy link
Contributor Author

Spone commented Oct 27, 2023

Sorry, I'm not really a VCR hardcore user

Me neither, and I always get a bit confused with the default behavior...

Okay, I'm getting there, but I still have an odd behavior:

When running with the scraper, I get the following stats:

{"plays" => nil, "likes" => nil, "comments" => nil}

But when running with the API:

{"plays" => nil}

Is there a way to expect different things depending on the service used? Or should I aim at having the same output?

@markets
Copy link
Collaborator

markets commented Oct 27, 2023

Yes! Both implementations are a bit different, since info is not available in both versions:

Scraper:

Api:

In the specs, you can use the api_key var to check what version is running:

[nil, ENV["VIMEO_ACCESS_TOKEN"] || "vimeo_access_token"].each do |api_key|

Something like this 👇🏼 is probably enough:

if api_key
  ...
else
  ...
end  

@Spone
Copy link
Contributor Author

Spone commented Oct 27, 2023

Thanks for your help! I think it's ready for review :)

EDIT: Damn, the specs pass locally but fail in Github Actions :/

EDIT 2: The failures are in Youtube tests, not sure how to fix those.

Copy link
Collaborator

@markets markets left a comment

Choose a reason for hiding this comment

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

Ah right! I think this is due to another problem we should improve some day. The point is that the secret ENV vars in our CI are not shared with forks (IIRC this is the default GH Actions behaviour).

But the changes seems fine to me, so lets merge it and see how it goes in master.

Thanks!!

@markets markets merged commit 52ac0a7 into thibaudgg:master Oct 27, 2023
0 of 6 checks passed
@Spone Spone deleted the safe-user-interaction-count branch October 27, 2023 21:31
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.

Vimeo Scraper: interactionStatistic can be missing
2 participants