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

Closes #72: Adds more test cases to feedParser.test.js #219

Closed
wants to merge 4 commits into from

Conversation

@nazneennahar
Copy link
Contributor

nazneennahar commented Nov 15, 2019

Appended my testcases at the end of the file since someone else made testcases.

@nazneennahar nazneennahar requested a review from humphd Nov 15, 2019
Copy link
Contributor

humphd left a comment

This is pretty good. I left a comment to fix all the uses of err.message to err.code. Also, in a follow-up Issue/PR, we should switch away from depending on the network to do real requests, since it can go down, URLs can change, etc.

try {
await feedParser('http://doesnotexists___.com');
} catch (err) {
expect(err.message).toBe('getaddrinfo ENOTFOUND doesnotexists___.com');

This comment has been minimized.

Copy link
@humphd

humphd Nov 15, 2019

Contributor

It's risky to depend on the error message like this. They can change between versions of the platform, different OSes can have slightly different error messages, error strings can be localized, etc.

Instead, use err.code and one of the official error codes

This comment has been minimized.

Copy link
@nazneennahar

nazneennahar Nov 15, 2019

Author Contributor

Thanks sir for your preview,I will be changing it right now.

This comment has been minimized.

Copy link
@nazneennahar

nazneennahar Nov 15, 2019

Author Contributor

I have made the changes.

Thanks

try {
await feedParser('http://doesnotexists___.com');
} catch (err) {
expect(err.code).toBe('getaddrinfo ENOTFOUND doesnotexists___.com');

This comment has been minimized.

Copy link
@humphd

humphd Nov 16, 2019

Contributor

Looks like you aren't running your own tests, since these don't pass. Why not?

npm test

You need to compare to the error code as a string:

expect(err.code).toBe('ENOTFOUND');

Copy link
Contributor

humphd left a comment

This is still failing tests, see https://circleci.com/gh/Seneca-CDOT/telescope/344?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link.

Also, you need to rebase on master and fix the merge conflict you have in the test file (more tests landed since you started this).

const nonFeedURL = 'https://kerleysblog.blogspot.com';
await feedParser(nonFeedURL);
} catch (err) {
expect(err.code).toBe('Not a feed');

This comment has been minimized.

Copy link
@humphd

humphd Nov 16, 2019

Contributor

Error codes have to be one of the E-------- strings I linked you, like ENOTFOUND. This won't work.

@manekenpix manekenpix added this to In progress/Review in Main via automation Nov 17, 2019
@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 20, 2019

@nazneennahar how is this going? Do you need any help?

@nazneennahar

This comment has been minimized.

Copy link
Contributor Author

nazneennahar commented Nov 21, 2019

@humphd - yes sir i do,i will show the issues i am facing tomorrow in class.

@nazneennahar nazneennahar force-pushed the nazneennahar:issue-72 branch from 2507a51 to 4edb350 Nov 21, 2019
@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 21, 2019

This needs some fixes for line endings and linting, but is pretty close.

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 22, 2019

I finished this up in #325, closing here.

@humphd humphd closed this Nov 22, 2019
Main automation moved this from In progress/Review to Closed Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Main
Closed
3 participants
You can’t perform that action at this time.