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.

Sign up for GitHub

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

Jump to bottom

ProjectRemixTest passes on ChromeHeadless #32520

Merged
merged 1 commit into from Jan 6, 2020
Merged

ProjectRemixTest passes on ChromeHeadless #32520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/src/code-studio/components/header/ProjectRemix.jsx
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ import PropTypes from 'prop-types';
import React from 'react';
import {connect} from 'react-redux';
import i18n from '@cdo/locale';

import * as utils from '../../../utils';
import {refreshProjectName} from '../../headerRedux';

class ProjectRemix extends React.Component {
Expand All @@ -21,7 +21,7 @@ class ProjectRemix extends React.Component {
) {
dashboard.project.serverSideRemix();
} else if (!this.props.isSignedIn) {
window.location.assign(
utils.navigateToHref(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

/**
* Wrapper for window.location.href which we can stub in unit tests.
* @param {string} href Location to navigate to.
*/
export function navigateToHref(href) {
if (!IN_UNIT_TEST) {
window.location.href = href;
}
}

And if you're wondering about the difference between location.assign(x) and location.href = x... apparently there is none.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

`/users/sign_in?user_return_to=${window.location.pathname}`
);
} else {
Expand Down
10 changes: 5 additions & 5 deletions apps/test/unit/code-studio/components/header/ProjectRemixTest.js
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ import {shallow} from 'enzyme';

import {expect} from '../../../../util/reconfiguredChai';
import {replaceOnWindow, restoreOnWindow} from '../../../../util/testUtils';

import * as utils from '@cdo/apps/utils';
import {UnconnectedProjectRemix as ProjectRemix} from '@cdo/apps/code-studio/components/header/ProjectRemix';

const defaultProps = {
Expand Down Expand Up @@ -54,18 +54,18 @@ describe('ProjectRemix', () => {
});

it('will redirect to sign in if necessary', () => {
sinon.stub(window.location, 'assign');
sinon.stub(utils, 'navigateToHref');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ChromeHeadless complains right here, because Chrome won't let you stub methods on its built-in Location object.


const wrapper = shallow(<ProjectRemix {...defaultProps} />);
wrapper.simulate('click');
expect(window.location.assign.calledOnce).to.be.true;
expect(utils.navigateToHref.calledOnce).to.be.true;
expect(
window.location.assign.calledWith(
utils.navigateToHref.calledWith(
'/users/sign_in?user_return_to=/context.html'
)
).to.be.true;

window.location.assign.restore();
utils.navigateToHref.restore();
});

it('will copy the project', () => {
Expand Down