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

Unit test and fixed bug #13

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

kketernality
Copy link

This pull request contains the following updates:

Unit Test

  1. Implemented unit test using Catch2 [1] and ported suitable tests from the original Python repository [2]. The unit test files locate in the test folder as before.
  2. Updated CMakeLists.txt and README.md to provide instruction of building and running unit test.

Fixed Bugs

  • [v] The example from Readme gave the wrong result. The cause is that the full_process is default on and will strip the tailing exclamation mark. It was fixed by altering the default value of full_process to false.
ReadmeExample
FAILED:
      REQUIRE( 97 == fuzz::ratio("this is a test", "this is a test!") );
EXPANSION:
      97 == 100 
  • [v] Comparison of two empty strings should give 100% similarity as in the original Python implementation. Fixed by adding additional logic in ratio() and partial_ratio().
RatioTest
      testEmptyStringsScore100
FAILED:
      REQUIRE( 100 == fuzz::ratio("", "") )
EXPANSION:
      100 == 0
  • [v] The partial_ratio() of two identical string is 0% similarity which should be 100% undoubtedly. The detailed description is in the fuzzywuzzy.cpp. This was fixed by a temporary solution to compare if two strings is identical or not.
RatioTest
      testPartialRatio
FAILED:
      REQUIRE( 100 == fuzz::partial_ratio("new york mets", "new york mets") )
EXPANSION:
      100 == 0

```
ReadmeExample (test_readme.cpp)

FAILED:
  REQUIRE( 97 == fuzz::ratio("this is a test", "this is a test!") );
EXPANSION:
  97 == 100
```

The `full_process` is default on and will strip the tailing exclamation mark.
It was fixed by altering the default value of `full_process` to false.
set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
```
RaioTest
  testEmptyStringsScore100

FAILED:
  REQUIRE( 100 == fuzz::ratio("", "") )
EXPANSION:
  100 == 0
```

Fixed by checking empty string in `fuzz::ratio` and `fuzz:partial_ratio`.
```
RatioTest
  testPartialRatio

FAILED:
  REQUIRE( 100 == fuzz::partial_ratio(s1, s1) )
EXPANSION:
  100 == 0
```

The `partial_ratio` of two identical strings should be 100 undoubtedly.
The detailed description of this problem is written in the `fuzzywuzzy.cpp`.
1) How to build and run tests
2) Enable command `make test`
@tmplt
Copy link
Owner

tmplt commented Mar 12, 2020

Thanks for taking the time to port unit tests over! I'll see about going over the commits the coming week.

@kketernality
Copy link
Author

kketernality commented Mar 13, 2020

Just let me know if there is anything goes wrong or needs to be updated. ^_^

Copy link
Owner

@tmplt tmplt left a comment

Choose a reason for hiding this comment

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

I'm not partial to adding a ~17kloc library header. Could you source catch.hpp from a submodule instead?

test/main.cpp Outdated Show resolved Hide resolved
test/test_fuzzywuzzy.cpp Show resolved Hide resolved
test/test_fuzzywuzzy.cpp Show resolved Hide resolved
test/test_fuzzywuzzy.cpp Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@kketernality
Copy link
Author

I'm not partial to adding a ~17kloc library header. Could you source catch.hpp from a submodule instead?

Okay, I think make it as an installed package or something likewise is more proper. Will be added in the next commit.

1) Catch2 libray now need to be installed or placed in include path to build successfully
2) Add comments to describle the commented out testcases in `test_fuzzywuzzy.cpp`
@kketernality
Copy link
Author

@tmplt

I had updated the required change. Please review the code changes in this new commit.

Copy link
Owner

@tmplt tmplt left a comment

Choose a reason for hiding this comment

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

Pardon the extended delay. Commits look good to me. Please rebase out test/catch.hpp from the commit history and I'll merge.

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.

2 participants