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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰Check hostname for default domain #17911

Merged
merged 3 commits into from Sep 7, 2018
Merged

馃悰Check hostname for default domain #17911

merged 3 commits into from Sep 7, 2018

Conversation

calebcordry
Copy link
Member

Before this change the linker was comparing against the full url instead of just the hostname.

@@ -177,6 +179,43 @@ describe('Linker Manager', () => {
});
});


it('should add linker with no defaultDomain and matching hostname', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

with no defaultDomain -> with default destinationDomains

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


return Promise.all(manager.allLinkerPromises_).then(() => {
manager.handleAnchorMutation(a);
return expect(a.href).to.not.equal('https://www.example.com');
Copy link
Contributor

Choose a reason for hiding this comment

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

compare to the full URL

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to avoid testing the linker in the linker-manager, but I agree it is probably better. Done.

extensions/amp-analytics/0.1/test/test-linker-manager.js Outdated Show resolved Hide resolved
@calebcordry calebcordry merged commit ac3ca04 into ampproject:master Sep 7, 2018
@calebcordry calebcordry deleted the default-domain branch September 7, 2018 20:27
Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
* check hostname

* clean up rebase

* tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants