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

Add unit test for video parse #442

Merged
merged 9 commits into from
Aug 1, 2024

Conversation

nkdengineer
Copy link
Contributor

Details

Related Issues

follow up PR: #440

Manual Tests

Linked PRs

@nkdengineer nkdengineer marked this pull request as ready for review July 26, 2024 09:22
@ZhenjaHorbach
Copy link

ZhenjaHorbach commented Jul 26, 2024

@nkdengineer
As far as I can see, the tests are broken
Can you check, please ?

@nkdengineer
Copy link
Contributor Author

I'll update today

@nkdengineer
Copy link
Contributor Author

Still investigating to find the reason the test failed.

@tomekzaw
Copy link
Collaborator

@nkdengineer The test with ![```test```](https://example.com/video.mp4) is failing because video alt text is escaped. Please come up with the fix for this input.

@nkdengineer
Copy link
Contributor Author

@tomekzaw Actually the video tag has the format <video>{videoName}</video> and then the other markdowns are applied in videoName. We need some changes in expensify-common to prevent other markdowns from being applied inside the video tag.

@nkdengineer
Copy link
Contributor Author

@tomekzaw I just added a patch about the required change in expensify-common let me know your thought.

@tomekzaw
Copy link
Collaborator

@nkdengineer Please re-build parser as the CI is failing.

@nkdengineer
Copy link
Contributor Author

@tomekzaw Maybe I built the parser with the expensify-common applied patch and then the lint check failed. Do you think we need a PR in expensify-common?

@tomekzaw
Copy link
Collaborator

Yes, please propagate all necessary changes to expensify-common.

Also, can you please run cd parser && yarn build and commit react-native-live-markdown-parser.js again?

@nkdengineer
Copy link
Contributor Author

Yes, please propagate all necessary changes to expensify-common.

My newest commit already runs build and đupate react-native-live-markdown-parser.js. The test fails maybe as I mentioned above. I think we can hold until I create a PR in expensify-common and it's merged.

Also, can you please run cd parser && yarn build and commit react-native-live-markdown-parser.js again?

@tomekzaw
Copy link
Collaborator

The test fails because react-native-live-markdown-parser.js generated on CI is different than the one that is committed.

Looks like you've added a patch in parser/ directory – but the patch file is not listed in package.json.

Also, we don't run yarn patch-package when installing dependencies on the CI.

That's why the CI is failing.

I think we can hold this PR until the PR to expensify-common is merged.

@nkdengineer
Copy link
Contributor Author

@tomekzaw @ZhenjaHorbach An expensify-common PR here.

Expensify/expensify-common#769

@nkdengineer
Copy link
Contributor Author

@tomekzaw The epxensify-common PR is merged and I bumped the version.

@ZhenjaHorbach
Copy link

@nkdengineer
Oh, nice job !
@tomekzaw
Looks like we are ready to move on !

@tomekzaw tomekzaw merged commit b0a5ce1 into Expensify:main Aug 1, 2024
3 checks passed
@tomekzaw
Copy link
Collaborator

tomekzaw commented Aug 1, 2024

@nkdengineer Thanks for the PR!

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

Successfully merging this pull request may close these issues.

3 participants