ProjectRemixTest passes on ChromeHeadless #32520
Conversation
| @@ -54,18 +54,18 @@ describe('ProjectRemix', () => { | |||
| }); | |||
|
|
|||
| it('will redirect to sign in if necessary', () => { | |||
| sinon.stub(window.location, 'assign'); | |||
| sinon.stub(utils, 'navigateToHref'); | |||
There was a problem hiding this comment.
ChromeHeadless complains right here, because Chrome won't let you stub methods on its built-in Location object.
| @@ -21,7 +21,7 @@ class ProjectRemix extends React.Component { | |||
| ) { | |||
| dashboard.project.serverSideRemix(); | |||
| } else if (!this.props.isSignedIn) { | |||
| window.location.assign( | |||
| utils.navigateToHref( | |||
There was a problem hiding this comment.
We fix by switching to our own navigateToHref function, which exists entirely for making scenarios like this testable.
code-dot-org/apps/src/utils.js
Lines 715 to 723 in 209b008
And if you're wondering about the difference between location.assign(x) and location.href = x... apparently there is none.
There was a problem hiding this comment.
Note that location.assign enforces same-origin domain (see the specs for location.href = and location.assign); are we confident that's not relevant here?
There was a problem hiding this comment.
Hey, I had no idea. Nice catch. I'm pretty sure it's not relevant here because we've hard-coded /users/sign_in into the call, which is an origin-relative path to begin with. The only thing I can think of is if there's some convoluted case with iframes, but window.location != window.parent.location so I think that's a non-issue too.
islemaster
changed the title
| @@ -21,7 +21,7 @@ class ProjectRemix extends React.Component { | |||
| ) { | |||
| dashboard.project.serverSideRemix(); | |||
| } else if (!this.props.isSignedIn) { | |||
| window.location.assign( | |||
| utils.navigateToHref( | |||
There was a problem hiding this comment.
Note that location.assign enforces same-origin domain (see the specs for location.href = and location.assign); are we confident that's not relevant here?
I've been trying to run tests in ChromeHeadless on my local machine, since PhantomJS isn't working properly on recent versions of Ubuntu. Eventually, I'd like us to use ChromeHeadless in our CI builds too, but to do that, we need to fix up all the tests that fail on this browser.
One of the tests that fails is
ProjectRemix#will redirect to sign in if necessary. I believe this fixes that test under both ChromeHeadless and PhantomJS.See also
Reviewer Checklist: