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

The chinese-normalization-pinyin feature flag doesn't compile #290

Closed
ManyTheFish opened this issue May 21, 2024 · 6 comments · Fixed by #291
Closed

The chinese-normalization-pinyin feature flag doesn't compile #290

ManyTheFish opened this issue May 21, 2024 · 6 comments · Fixed by #291
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@ManyTheFish
Copy link
Member

Reproduction

cargo test --verbose --features chinese-normalization-pinyin

Fix

  1. Fix the CI testing the flag using cargo test --verbose --features chinese-normalization-pinyin instead of cargo test --verbose --features chinese chinese-normalization-pinyin
  2. the CI should not work anymore
  3. Fix the compilation issue in the normalizer

related to meilisearch/meilisearch#4629

@Soham1803
Copy link
Contributor

Just to be clear, what's expected here is the new CI command i.e. cargo test --verbose --features chinese-normalization-pinyin instead of the older cargo test --verbose --features chinese chinese-normalization-pinyin which currently runs fine. Am I right?
When currently we run cargo test --verbose --features chinese-normalization-pinyin total 102+9 tests run whereas we expect only one test related to chinese normalizer to run.

@ManyTheFish
Copy link
Member Author

ManyTheFish commented May 23, 2024

I made a mistake when putting cargo test --verbose --features chinese chinese-normalization-pinyin because in this case the flag chinese-normalization-pinyin is ignored, the proper command to run would be cargo test --verbose --features chinese,chinese-normalization-pinyin.
But cargo test --verbose --features chinese-normalization-pinyin is sufficient.
The goal is to ensure that the code under this flag is tested and works. Currently it doesn't.

@Soham1803
Copy link
Contributor

Soham1803 commented May 26, 2024

Hey @ManyTheFish! Waiting for your review to this above PR by @tkhshtsh0917. I actually doubt whether that's the complete solution. Because even after that if you run cargo test --verbose --features chinese-normalization-pinyin it executes all 102+9 tests which is what happens when you simply run cargo test. I might be wrong about this. I'm sorry, I'm just curious and want to know more about this project.

@tkhshtsh0917
Copy link
Contributor

tkhshtsh0917 commented May 26, 2024

Hi @Soham1803! I would like to properly understand your thoughts, are you arguing that we should use a trick where only the features specified in cargo test --features are tested and other features are skipped?

Like this?
ref: https://stackoverflow.com/questions/77529267/how-to-run-only-tests-marked-with-a-specific-feature-and-ignoring-the-rest

@ManyTheFish
Copy link
Member Author

Hello @Soham1803 and @tkhshtsh0917, you are right. The number of tests doesn't change; however, they are not exactly the same. In the Chinese normalizer tests the expected result change depending on the feature flag. Whit your PR, both sides will be tested.

@Soham1803
Copy link
Contributor

Oh yes! I get it now. Thanks @ManyTheFish for clearing my doubt and all the best @tkhshtsh0917 with your PR. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants