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

Feature/122 unit tests for strings #429

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

Conversation

harshit496
Copy link
Contributor

No description provided.

@harshit496 harshit496 linked an issue Nov 5, 2024 that may be closed by this pull request
@harshit496 harshit496 marked this pull request as ready for review November 5, 2024 08:22
Copy link
Owner

@aregtech aregtech left a comment

Choose a reason for hiding this comment

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

Please read my comments.

* Tests makeInt64() which converts a string
* to a int64
*/
TEST(StringTest, TestMakeInt64)
Copy link
Owner

Choose a reason for hiding this comment

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

Would be nice to add a case for negative number and positive number with the sign +.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have used the Gtest parameterize feature to parameterize the test in this test case. please take a look if that works for you and I can modify other test cases accordingly. thx

* to a unsigned int64
*/
TEST(StringTest, TestMakeUInt64)
{
Copy link
Owner

Choose a reason for hiding this comment

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

Would be also nice to add a case, where the uint64_t is greater or equal to 0x8000'0000'0000'0000 to make sure that the 64-bit conversion is correct.

* to type int32.
*/
TEST(StringTest, TestMakeInt32)
{
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please add following cases:

  • negative number conversion;
  • positive number with + sign, like constexpr char * test = "+123";
  • Other non-standard cases like:
    • constexpr char * test = "abcd";
    • constexpr char * test = "123abc";
    • constexpr char * test = "abc123:'
  • Cases with various radix:
    typedef enum class E_Radix : int8_t
    {
          RadixAutomatic    =  0    //!< Detect automatically
        , RadixBinary       =  2    //!< Binary conversion, conversion base is 2
        , RadixOctal        =  8    //!< Octal conversion, conversion base is 8
        , RadixDecimal      = 10    //!< Decimal conversion, conversion base is 10
        , RadixHexadecimal  = 16    //!< Hexadecimal conversion, conversion base is 16
    } eRadix;
  • Please also check the value of the end parameter after calling makeXXX. Here is the syntax:
     /**
      * \brief   Converts given string of digits to 32-bit integer
      * \param   strDigit    The string with digits. Can contain negative or positive sign in front
      * \param   radix       The base value when calculate integer.
      * \param   end [out]   If not nullptr, on output this contains value of pointer to the next character in strDigit buffer after the numerical value.
      * \return  Returns 32-bit integer
      **/
     static int32_t makeInt32( const char * strDigit, NEString::eRadix radix = NEString::eRadix::RadixDecimal, const char ** end = nullptr );

Ideally, would be nice to have parameterized tests to add various parameters and check outputs. Checkout the parameterized test example of strings in NEStringTest.cpp. Same is for other makeXXX tests.

I indeed appreciate very much your help, but then I'll have to rework and nearly overwrite everything to make tests proper :)

@aregtech
Copy link
Owner

aregtech commented Nov 9, 2024

Thank you! Looks much better.

@aregtech aregtech self-assigned this Nov 9, 2024
@harshit496
Copy link
Contributor Author

Can be merged?

@aregtech
Copy link
Owner

aregtech commented Nov 9, 2024

Could you please make sure that it is compiled under Visual Studio? I tried locally and get errors.

  1. The .cpp file is not included in the areg-unit-tests.vcxproj and areg-unit-tests.vcxproj.filters files;
  2. The MSVC google test adapter does not understand INSTANTIATE_TEST_SUITE_P macro. In other files I use
    #if defined(INSTANTIATE_TEST_SUITE_P)
    INSTANTIATE_TEST_SUITE_P()
    #else   // !defined(INSTANTIATE_TEST_SUITE_P)
    INSTANTIATE_TEST_CASE_P()
    #endif  // defined(INSTANTIATE_TEST_SUITE_P)
    An example see in file NEStringTest.cpp

The builds in this PR passed, because new StringTest.cpp file is not included in MSVC build. As soon as you include, the MSBuild will fail.
@harshit496 , can you please fix the issue?

@harshit496
Copy link
Contributor Author

Ah okay. I will take a look. Thanks for pointing out

@harshit496
Copy link
Contributor Author

@aregtech I forgot to mention that I am using Linux and I was able to build it fine with cmake although it wont be able to build using msbuild right since that is windows specific?

@aregtech
Copy link
Owner

@harshit496, there is GitHub action workflow to run MSBuild. If you include the file in the MSVS project, it will fail.
Another solution is, if you merge changes not with main, but with other branch, then I can work on that and fix. Let me know if you need a branch.

@harshit496
Copy link
Contributor Author

@aregtech I added it to MS build project. where can I find the github action workflow link to see it fail? because currently under the checks tabs on this PR it does not run any jobs. i dont know why

@aregtech
Copy link
Owner

Hi @harshit496 ,

This the workflow. Your fork should also have it. As soon as you merge with your master branch, it will be triggered.

Alternatively, you can edit the beginning of msbuild.yml to compile on every push and pull request on any branch:

name: MSBuild

on:
  push:         # Keep empty to run on each branch when push the code. Otherwise, use branches: [ master ]
    # branches: [ master ]
  pull_request:
    # branches: [ master ]

Then you can test locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Unit Tests for strings
2 participants