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 test for the Issue#495 #546

Closed
wants to merge 1 commit into from
Closed

Conversation

Reqrefusion
Copy link

I'm adding these tests in case it improves in the future. @likemusic explained how this problem got there in #495.

I'm adding these tests in case it improves in the future. @likemusic explained how this problem got there in smalot#495
@k00ni
Copy link
Collaborator

k00ni commented Jul 4, 2022

@Reqrefusion Thank you for your pull request. I would like to merge it right away, but tests fail.

@Reqrefusion
Copy link
Author

Hello, it should not pass tests anyway because it cannot parser the relevant documents correctly. It will pass this test when you do it correctly. So technically, it shouldn't have passed this test anyway. I designed this test this way, but if you say it should give an error in case of a change, I can understand. But at that time it will throw an error when an output is true. Honestly, it's a bit of a complicated problem. After all, this is not a fix but a forward-looking reminder that a problem cannot be solved. The main question is whether these reminders will be made through unit test. I'm asking you, @k00ni how it should be. I'll edit the PR based on your answer.

Copy link
Collaborator

@k00ni k00ni left a comment

Choose a reason for hiding this comment

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

Hello @Reqrefusion, thank you for your clarification. I follow your argument.

The main question is whether these reminders will be made through unit test. I'm asking you, @k00ni how it should be. I'll edit the PR based on your answer.

I suggest the following:

  • please split up your new test into 3, each for one PDF file. This way we can assure that there is no mixture later on, when related changes arrive into the code.
  • add $this->expectExceptionMessage('Some error message'); at the beginning of each test, with an appropriate message to catch the exception/error which is currently thrown (iconv(): Detected an illegal character in input string). Hope that is technically possible.
  • add a short explanation why we expect this behavior, plus a reference to this PR and the issue The ö, ü and ç characters sometimes appear as � #495

Thanks.

@k00ni
Copy link
Collaborator

k00ni commented Aug 12, 2022

@Reqrefusion Can you tell where we stand here?

@k00ni
Copy link
Collaborator

k00ni commented Sep 9, 2022

Closed for now due to inactivity, please reopen when you want to work on it again.

@k00ni k00ni closed this Sep 9, 2022
@Reqrefusion
Copy link
Author

@Reqrefusion Can you tell where we stand here?

Oh sorry I didn't see it. I'm busy these days, but I'll be back for sure sometime. Thank you.

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

Successfully merging this pull request may close these issues.

2 participants