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 some tests to translateFile #10

Merged
merged 3 commits into from
Oct 31, 2021
Merged

Conversation

pasagedev
Copy link
Contributor

Added some test for translateFile function.
referred issue: #2

Checklist

  • add translateFile test for Google Translator
  • add translateFile test for MyMemory Translator

@pasagedev pasagedev mentioned this pull request Oct 30, 2021
@nidhaloff nidhaloff added good first issue Good for newcomers Hacktoberfest hacktoberfest label labels Oct 31, 2021
@nidhaloff
Copy link
Owner

@pasagedev thanks for the PR. Did you run the tests locally first? if not try it by running npm run test and checking the output.

Please notice that translators behave differently, for example, one test is failing because:
MyMemory translate to this --> Hello you are great, keep up the good work
google to this --> Hello, you are great, keep it up

Therefore, you need to update the expected result in the test or try a simpler example just for test purposes (e.g just "how are you")

If you fix this issue, I will accept the PR ;)

@pasagedev
Copy link
Contributor Author

@pasagedev thanks for the PR. Did you run the tests locally first? if not try it by running npm run test and checking the output.

Please notice that translators behave differently, for example, one test is failing because: MyMemory translate to this --> Hello you are great, keep up the good work google to this --> Hello, you are great, keep it up

Therefore, you need to update the expected result in the test or try a simpler example just for test purposes (e.g just "how are you")

If you fix this issue, I will accept the PR ;)

Of course I did but I don't know why tests were passing. However I did run tests again and detect that you said and fixed it.

@nidhaloff
Copy link
Owner

@pasagedev hmm that's odd, the tests still fail when running on github actions. You can check the output here https://github.com/nidhaloff/dolmetscher/runs/4060428910?check_suite_focus=true

@pasagedev
Copy link
Contributor Author

@pasagedev hmm that's odd, the tests still fail when running on github actions. You can check the output here https://github.com/nidhaloff/dolmetscher/runs/4060428910?check_suite_focus=true

Yes it's odd. I saw results of github actions tests but I note than in last commit I did, it not be reflected there because the expected result should be "Hello you are great keep up the good work" just you can see in: last commit.

@nidhaloff
Copy link
Owner

@pasagedev maybe you can use something simpler. For example "how are you" or a very simple example, even one word.

@pasagedev
Copy link
Contributor Author

@pasagedev maybe you can use something simpler. For example "how are you" or a very simple example, even one word.

I did it, I hope than it works now :)

@nidhaloff
Copy link
Owner

@pasagedev It turns out mymemory is translating hallo to hi and google translator hallo to hello :D it is quite confusing. Anyway I will just accept your PR since you put much work in it and I will try to fix this

@nidhaloff nidhaloff merged commit dd20611 into nidhaloff:master Oct 31, 2021
@pasagedev
Copy link
Contributor Author

@pasagedev It turns out mymemory is translating hallo to hi and google translator hallo to hello :D it is quite confusing. Anyway I will just accept your PR since you put much work in it and I will try to fix this

Thank you and sorry for my mistakes. I don't know exactly why tests pass on local but it fails on github actions.

@pasagedev pasagedev deleted the add-some-test branch October 31, 2021 16:03
@pasagedev
Copy link
Contributor Author

@nidhaloff can you help me to complete my hacktoberfest goals tagging this PR with "hacktoberfest-accepted" ? 🙂

@nidhaloff
Copy link
Owner

@pasagedev Sure ;)

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

Successfully merging this pull request may close these issues.

2 participants