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

Fix #115: added test cases for sentiment analysis #207

Merged
merged 15 commits into from Nov 15, 2019
Merged

Fix #115: added test cases for sentiment analysis #207

merged 15 commits into from Nov 15, 2019

Conversation

dbeigi
Copy link
Contributor

@dbeigi dbeigi commented Nov 15, 2019

This pull request is to address #115 . I wrote two test cases that will test if sentiment analysis functionality will a) return the unexpected object b) return the expected object.

@jatinAroraGit would love to see your feedback on the following tests.

@dbeigi dbeigi added the type: test Creation and development of test label Nov 15, 2019
@dbeigi dbeigi closed this Nov 15, 2019
@dbeigi dbeigi reopened this Nov 15, 2019
@manekenpix manekenpix added this to In progress/Review in Main via automation Nov 15, 2019
Copy link
Member

@manekenpix manekenpix left a comment

Choose a reason for hiding this comment

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

Looking good. Just a couple of suggestions, feel free to dismiss them.

test/sentiment-analysis.test.js Outdated Show resolved Hide resolved
test/sentiment-analysis.test.js Outdated Show resolved Hide resolved
Copy link
Member

@manekenpix manekenpix left a comment

Choose a reason for hiding this comment

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

Looks good to me, but you have a bunch of commits that are not related to this PR. You can try to get rid of them:

Screenshot from 2019-11-15 00-47-22


test('test if sample text does not return the correct object', async () => {
const data = await sentimentAnalysis.run('I like dogs, but my girlfriend is afraid of them');
expect(data).toEqual(expect.not.objectContaining(expected));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you not just compare data to expected with a deep equals here? What does .toEqual(expect.not.objectContaining(expected)) mean? Maybe you're right, but this seems odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@humphd From what I have read, .toEqual should be used for objects. Regards to expect.not.objectContaining(expected) , I am using this to check if the contents(properties) of the object do not match what we are expecting. This was suggested by @jatinAroraGit and after reading the docs I agreed to his suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, OK, sounds fine. Thanks for educating me.

@humphd humphd merged commit ae6dc42 into master Nov 15, 2019
Main automation moved this from In progress/Review to Done Nov 15, 2019
@humphd humphd deleted the issue115 branch November 15, 2019 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: test Creation and development of test
Projects
No open projects
Main
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants