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

Fixes #83: test case for text-parser #109

Merged
merged 5 commits into from Nov 14, 2019

Conversation

giatuongtran9
Copy link
Contributor

Adding test case for parsing html to text, reference to issue #75

@PavanKKamra, apparently JSOM does not support innetText, based on this Issue. I have tested it and innerHTML and textContent works fine but innerText will return 'undefined'

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Excellent research on this. I hadn't known about the innerText issue in jsdom. I agree that we can probably just use textContent instead.

This looks like a good start, but can you update your tests to do a few things:

  1. include some HTML that would produce newlines (e.g., h1 and p) and make sure they work as we'd expect.
  2. assume that textParser.run() returns a Promise vs. something we get back directly.

humphd
humphd previously approved these changes Nov 9, 2019
@giatuongtran9
Copy link
Contributor Author

Hi @humphd , I have change the test to return a Promise by async await.

I have noticed that dialogue.js has some errors whenever I run the test 'npm run test'
Screen Shot 2019-11-08 at 7 35 31 PM

@humphd
Copy link
Contributor

humphd commented Nov 9, 2019

@giatuongtran9 there is a PR to fix that in #106 if you want to review.

@Reza-Rajabi Reza-Rajabi added this to In progress/Review in Main via automation Nov 9, 2019
Copy link
Contributor

@cindyledev cindyledev left a comment

Choose a reason for hiding this comment

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

PR #106 fixes the linting errors in the dialogue.js file so you just need to rebase and resolve the path to module

Annotation 2019-11-09 172625

@humphd
Copy link
Contributor

humphd commented Nov 10, 2019

@giatuongtran9 it looks like you've merged and pulled in a bunch of unrelated changes. Also, there are merge conflict markers in test/text-parser.test.js.

NOTE: you want to rebase but not merge with master.

@giatuongtran9
Copy link
Contributor Author

@humphd I made some mistakes at the beginning and try to rebase to master but somehow pulled all the new files. The test fail because we don't have the text-parser.js yet

@humphd
Copy link
Contributor

humphd commented Nov 10, 2019

OK, we'll have to fix this git history. I can help you when we see each other at the start of the week, or you can try and do it yourself between now and then.

As far as the failure, tell Jest to skip these tests until we have an implementation: https://jestjs.io/docs/en/api.html#testskipname-fn

@giatuongtran9
Copy link
Contributor Author

I will try to fix this but I think I will need your help. skip cannot be used here because the test need the run method from text-parser.js so we can comment out the test or do this for now:

expect('Hello World').toBe('Hello Word');

@humphd
Copy link
Contributor

humphd commented Nov 10, 2019

OK @giatuongtran9, how about this. Since @PavanKKamra doesn't seem to have started his work yet, why don't you add text-parser.js and have it export a function at least. In the code, you can leave a comment to the Issue that will implement it in #75. Then @PavanKKamra can work from there.

@PavanKKamra how is your work coming? It sounds like we're just about ready for your code here...

@giatuongtran9
Copy link
Contributor Author

@humphd I have added simple function for text-parser and @PavanKKamra can continue from there

src/text-parser.js Outdated Show resolved Hide resolved
src/text-parser.js Outdated Show resolved Hide resolved
test/text-parser.test.js Show resolved Hide resolved
src/text-parser.js Outdated Show resolved Hide resolved
humphd
humphd previously approved these changes Nov 12, 2019
Copy link
Contributor

@humphd humphd 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.

@humphd
Copy link
Contributor

humphd commented Nov 14, 2019

@giatuongtran9 you need to regenerate package-lock.json like so:

npm install
git add package-lock.json

@giatuongtran9
Copy link
Contributor Author

@humphd Thank you and @cindyledev please review it again

humphd
humphd previously approved these changes Nov 14, 2019
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Good stuff.

@giatuongtran9
Copy link
Contributor Author

@cindyledev please review again, if you left it here, other stuff will get merged and this will forever get conflict and never get merge
@humphd sorry for asking to review it again so many time

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.

There's a big amount of unrelated changes here. Try to get rid of them.

Tuong Tran and others added 5 commits November 14, 2019 12:17
author Tuong Tran <tuongtran@Tuongs-MacBook-Pro.local> 1573257565 -0500
committer Tuong Tran <tuongtran@Tuongs-MacBook-Pro.local> 1573498166 -0500

Fix Seneca-CDOT#83: Change test to async await
author Tuong Tran <tuongtran@Tuongs-MacBook-Pro.local> 1573257565 -0500
committer Tuong Tran <tuongtran@Tuongs-MacBook-Pro.local> 1573498166 -0500

Fix Seneca-CDOT#83: Change test to async await
@humphd humphd merged commit cab8076 into Seneca-CDOT:master Nov 14, 2019
Main automation moved this from In progress/Review to Done Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Main
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants