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

Strip domain from <base href> in useBasename #139

Merged
merged 2 commits into from
Nov 14, 2015
Merged

Strip domain from <base href> in useBasename #139

merged 2 commits into from
Nov 14, 2015

Conversation

jnv
Copy link
Contributor

@jnv jnv commented Nov 13, 2015

Resolves #138.

I have added a few checks for location.basename in tests and exposed extractPath; I hope it's fine.

)
if (!fullUrl) {
warning(
false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just pass in fullUrl here instead of false instead of adding the explicit branch.

@taion
Copy link
Contributor

taion commented Nov 13, 2015

Please squash your commits.

@jnv
Copy link
Contributor Author

jnv commented Nov 13, 2015

Gotcha.

@mjackson
Copy link
Member

Just FYI- I have temporarily disabled test builds on Travis because we're experiencing problems with Browserstack. I'm working on getting it back up now.

@knowbody
Copy link
Contributor

thanks for that @mjackson was going to ping you with that

@mjackson
Copy link
Member

I'm working with Browserstack support now. I hate how fragile the whole "test your code in real browsers" landscape is. I would've thought we'd have good solutions for this by now.

@acdlite
Copy link
Contributor

acdlite commented Nov 13, 2015

@mjackson Have you tried Sauce Labs? https://saucelabs.com/opensauce/

@mjackson
Copy link
Member

@acdlite Ya, I tried them when I was first getting started with this stuff. Seemed like Browserstack was the better choice at that time. It's probably more or less the same story over there. Just a different set of problems. Have you tried them?

@taion
Copy link
Contributor

taion commented Nov 13, 2015

I've done a little bit with SauceLabs before. I've seen vague flakiness, but nothing like a multi-day outage. Two cool Sauce Labs perks, though.

@geekdave
Copy link

@mjackson At WalmartLabs, we just resigned ourselves to the notion that e2e testing is a flakey ecosystem. And so we built some anti-flake tools to work around it. Currently we only support SauceLabs, but perhaps it's time we wrote an adapter for BS too.

Project page: http://testarmada.github.io/
Blog about the philosophy we're going for: https://medium.com/@geek_dave/zombies-and-soup-e346f0c8064f

</shameless-plug>

@ryan-roemer
Copy link

@mjackson -- I've got a number of test-focused repos (teaching and infrastructure) and have anecdotally found SL to be much more reliable than BS. Of course, that's just my isolated experiences, but for example, I maintain an OSS selenium + mocha + WD.js / Webdriverio bridge tool, and just ended up giving up and disabling BS in CI: FormidableLabs/rowdy#34

BS is very responsive to support requests, but I couldn't manage the flake sufficiently to keep it "on" for my repos.

@taion
Copy link
Contributor

taion commented Nov 13, 2015

@geekdave How compatible is the Armada approach with something like an open source SL plan that limits the number of parallel executions?

@Maciek416
Copy link

@taion you can simply limit your suite to match the number of workers you get from SL. We're hoping that one day we can convince them to increase that limit for open source projects, if we see more usage of our tool :) (the argument being that our tool encourages / brings about "better" test suites that make more effective use of the saucelabs cloud than tests that just sit around and time out a lot)

@geekdave
Copy link

@taion TestArmada has two approaches to handle that.

  1. A flag max_workers that can be applied to an individual build. You can set this to be equal to the number of VMs you have available.
  2. A new project called locks (in the nautical sense) that serves as a "traffic controller" for multiple builds contending for the same remote resources. Locks monitors the remote usage on the SL side and assigns "free slots" to builds waiting for VMs.

@jnv
Copy link
Contributor Author

jnv commented Nov 13, 2015

There is also https://ci.testling.com/ while you are at it. 😉
From contributor's point of view, both SauceLabs and Testling provide a free tier for opensource projects, while BrowserStack offers only a limited trial. This makes crossbrowser testing harder for anyone wishing to test their fork properly.

Anyway, this discussion went a bit astray. Would you like to perhaps start a new issue?

@mjackson
Copy link
Member

Seems like testing issues are resolved.

Can you please try a force push to your branch? That should trigger a
rebuild of the tests.

Thanks.

On Fri, Nov 13, 2015 at 11:15 AM Jan Vlnas [email protected] wrote:

There is also https://ci.testling.com/ while you are at it. 😉
From contributor's point of view, both SauceLabs and Testling provide a
free tier for opensource projects, while BrowserStack offers only a limited
trial. This makes crossbrowser testing harder for anyone wishing to test
their fork properly.

Anyway, this discussion went a bit astray. Would you like to perhaps start
a new issue?


Reply to this email directly or view it on GitHub
#139 (comment).

@mjackson
Copy link
Member

@geekdave Wow, test armada looks great! I'll def give it a go next time I need to setup something like this. Thanks for chiming in here :)

@mjackson
Copy link
Member

Actually, @jnv, you'll have to rebase and then force push to your branch. That way you'll get the new test config.

@jnv
Copy link
Contributor Author

jnv commented Nov 13, 2015

Okay, I had to change <base> element cleanup in tests but otherwise it seems to pass just fine.

jnv added 2 commits November 14, 2015 00:55
- Additional tests for location.basename
- Export extractPath, optionally accepting full URL
@jnv
Copy link
Contributor Author

jnv commented Nov 13, 2015

@mjackson I have changed the PR according to your comments. Should I squash the new commit?

mjackson added a commit that referenced this pull request Nov 14, 2015
Strip domain from <base href> in useBasename
@mjackson mjackson merged commit c2f91e9 into remix-run:master Nov 14, 2015
@mjackson
Copy link
Member

Looks great, @jnv. I'll take it from here. Thank you!

mjackson added a commit that referenced this pull request Nov 14, 2015
@jnv jnv deleted the base-href branch November 14, 2015 06:41
jnv added a commit to cvut/fittable that referenced this pull request Nov 15, 2015
Update to history was required due to remix-run/history#139
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants