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

Cleanup: remove pytest and online testing mode #659

Merged
merged 2 commits into from Oct 20, 2022

Conversation

jayaddison
Copy link
Collaborator

@jayaddison jayaddison commented Oct 20, 2022

The only use case for pytest in the codebase was for online scraper testing, and this wasn't a feature that we had tested since it was introduced.

In combination with the responses library, it doesn't currently function particularly well for any site that performs HTTP redirects. There are ways to workaround that, but I think that online testing requires a more thorough design and integration with CI if we're to offer it as a well-supported feature.

Until then, this pull request suggests removing pytest and the online testing functionality.

Relates to #617.

It turns out that 'online' mode is kinda broken at the moment in the library - that's my fault for introducing it without ongoing testing.  In particular, the 'passthrough' flag -- by-design in 'responses' -- functions on in individual HTTP requests.  So, if an online request is redirected by the server, then -- without further configuration -- responses won't match the subsequent request to the redirected URL.

Although some workarounds are possible, I think this feature needs a redesign.  It would be brilliant to support and improve near-current-data testing against various sites without sending them repeat/spammy request traffic.  'responses' has introduced a beta recording feature, and also VCR.py is available.
@jayaddison
Copy link
Collaborator Author

@hhursev @bfcarpio @astappiev does this approach seem OK to you?

I had -- still have -- grand plans for online testing (I think it'd be brilliant if we could keep up-to-date with recent website changes while minimizing request traffic and also adding a kind of safety-check to ensure site data is public) -- but I don't think the feature as-implemented was helping towards that, and until then it's probably better to remove it.

@jayaddison
Copy link
Collaborator Author

Perhaps it's brash, but I'm going to move ahead with this as-is. It's pure dependency/feature removal, and for a feature that already seems partially broken. Please feel free to disagree and I'll take another look.

@jayaddison jayaddison merged commit 5501a43 into main Oct 20, 2022
@jayaddison jayaddison deleted the issue-617/cleanup-remove-pytest branch October 20, 2022 21:47
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.

None yet

1 participant