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

e2e: reintroduce e2e test for providers locally #1706

Merged
merged 2 commits into from
Jul 29, 2019

Conversation

ifedapoolarewaju
Copy link
Contributor

No description provided.

@goto-bus-stop
Copy link
Contributor

this will need a big rebase after #1675 :/

@goto-bus-stop
Copy link
Contributor

The main change in the new wdio is that you'll need lots of awaits everywhere. Basically everything that does:

browser.someMethod('selector')

now needs to be:

const el = await browser.$('selector')
await el.someMethod()

@ifedapoolarewaju
Copy link
Contributor Author

@goto-bus-stop ready for a review now 👍

@@ -8,7 +8,7 @@ const Dropbox = require('@uppy/dropbox')
const Tus = require('@uppy/tus')

const isOnTravis = !!(process.env.TRAVIS && process.env.CI)
const companionUrl = isOnTravis ? 'http://companion.test:3030' : 'http://localhost:3030'
const companionUrl = isOnTravis ? 'http://companion.test:3030' : 'http://localhost:3020'
Copy link
Contributor

Choose a reason for hiding this comment

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

wdio runs Companion on :3030. can't we use that here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I resorted to 3020 for the following reasons:

  1. Oauth Redirect URI for the Providers in dev env is currently set to allow only http://localhost:3020
  2. I thought since this is meant to run locally there's no need to duplicate the usage of secrets nor is there a need to bother running a different companion instance.

I think the companion at 3030 serves for url-plugin on travis which does not need Oauth dance, also it made sense to have it because the other providers were intended to run on travis, but unfortunately, that's not the case anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@goto-bus-stop any reservations on this?

Copy link
Contributor

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

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

I would still like it if we could somehow use the wdio-run companion in the future but this LGTM now 👍

@ifedapoolarewaju ifedapoolarewaju merged commit b6085ed into master Jul 29, 2019
@delete-merged-branch delete-merged-branch bot deleted the providers-e2e-again branch July 29, 2019 07:44
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