-
Notifications
You must be signed in to change notification settings - Fork 401
chore(tests): Add unit tests for each component. #138
Conversation
Looks like CI is failing on an scss issue |
0827c7e
to
bff567e
Compare
I believe the scss issue was due to it building with |
466fdf1
to
717a1f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few tests that I think we can cut.
package.json
Outdated
@@ -21,7 +21,7 @@ | |||
"@angular/core": "^2.4.6", | |||
"@angular/forms": "^2.4.6", | |||
"@angular/http": "^2.4.6", | |||
"@angular/material": "^2.0.0-beta.1", | |||
"@angular/material": "^2.0.0-beta.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can revert this line now that we're on beta.3
fixture.detectChanges(); | ||
})); | ||
|
||
it('should render four rows of content on the homepage', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need these homepage tests since it's all just static content.
src/app/shared/footer/footer.spec.ts
Outdated
.querySelector('.docs-footer-links a'); | ||
const href = link.getAttribute('href'); | ||
const text = link.textContent; | ||
expect(~href.indexOf('angular.io')).toBeTruthy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to do
expect(href).toContain('angular.io');
src/app/shared/footer/footer.spec.ts
Outdated
expect(text).toContain('Learn Angular'); | ||
}); | ||
|
||
it('should show an angular logo in the footer', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a test for the this is necessary
src/app/shared/navbar/navbar.spec.ts
Outdated
fixture.detectChanges(); | ||
})); | ||
|
||
it('should have four main links', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test also isn't needed
b85ea7a
to
616a64d
Compare
616a64d
to
48b3094
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.