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 tests to check the order of torrent events #1739

Merged
merged 1 commit into from Sep 8, 2019

Conversation

@alxhotel
Copy link
Member

alxhotel commented Sep 7, 2019

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[ ] New feature
[X] Other, please explain: Tests

What changes did you make? (Give an overview)

This PR adds tests for #1737

Is there anything you'd like reviewers to focus on?

For #1737
@feross
feross approved these changes Sep 8, 2019
Copy link
Member

feross left a comment

This is excellent! Thanks!!

@feross feross merged commit 84fb338 into webtorrent:master Sep 8, 2019
3 checks passed
3 checks passed
LGTM analysis: JavaScript No new or fixed alerts
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alxhotel

This comment has been minimized.

Copy link
Member Author

alxhotel commented Sep 9, 2019

I just realised that the build fails because the tests don't pass in the browser, although I'm not sure why the browser tests weren't executed in the PR checks...

Anyhow, this line does not work in the browser (obviously):

torrent.addPeer('127.0.0.1:' + client2.address().port)

And it must be moved inside the infoHash handler.

I guess there are two options:

  1. Move the test file to the node folder. And only check the order of events in Node.
  2. Create a way to add a WebRTC peer to the torrent with: torrent.addPeer(webrtcPeer). There's an issue (#1574) about how to do this manually. But I'm not sure if the implementation shown there is the proper way to do it.

This brings me to another question: why aren't there any tests for downloading and seeding in the browser? Or am I missing something?

@feross

This comment has been minimized.

Copy link
Member

feross commented Sep 10, 2019

A lot of the tests are node-only. I think we can solve this by moving the test to the node folder for now. This is still a lot better than not having this test. We can figure out how to solve the webtrc peer issue separately.

We don't run the browser tests in PRs since there is a secret API keys required to interact with Saucelabs and travis doesn't expose those environment variables to PRs since then anyone could send a PR and just log out the secret API key. It's not ideal, but it makes some sense.

@alxhotel

This comment has been minimized.

Copy link
Member Author

alxhotel commented Sep 10, 2019

We don't run the browser tests in PRs since there is a secret API keys required to interact with Saucelabs and travis doesn't expose those environment variables to PRs since then anyone could send a PR and just log out the secret API key. It's not ideal, but it makes some sense.

Ah got it 👍

We can figure out how to solve the webtrc peer issue separately.

Great!

I think we can solve this by moving the test to the node folder for now

Thanks for fixing this 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.