nnstreamer / nntrainer

NNtrainer is Software Framework for Training Neural Network Models on Devices.
Apache License 2.0
135 stars 71 forks source link

Use parameterized test in `AppContextTest` #2533

Closed Boseong-Seo closed 3 months ago

Boseong-Seo commented 3 months ago

To make code more readable, use parameterized test according to existing TODO comment.

Self evaluation:

  1. Build test: [X]Passed [ ]Failed [ ]Skipped
  2. Run test: [X]Passed [ ]Failed [ ]Skipped

Signed-off-by: Boseong Seo suzy13549@snu.ac.kr

taos-ci commented 3 months ago

:memo: TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2533. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/.

Boseong-Seo commented 3 months ago

@jijoongmoon @DonghakPark As you commented, I tried to make this PR more concisely by removing some commits and doing rebase. I would appreciate it if you could let me know if there's anything else that needs to be modified!

DonghakPark commented 3 months ago

@jijoongmoon @DonghakPark As you commented, I tried to make this PR more concisely by removing some commits and doing rebase. I would appreciate it if you could let me know if there's anything else that needs to be modified!

👍 i will run all CI check & our member will review your commit. for now i can see static checkers & verifiers fail about clang-format

The clang-format complaints...
  diff --git a/test/unittest/unittest_nntrainer_appcontext.cpp b/test/unittest/unittest_nntrainer_appcontext.cpp
  index 659a29f5..f385070a 100644
  --- a/test/unittest/unittest_nntrainer_appcontext.cpp
  +++ b/test/unittest/unittest_nntrainer_appcontext.cpp
  @@ -163,[30](https://github.com/nnstreamer/nntrainer/actions/runs/8549302966/job/23427159699?pr=2533#step:5:31) +163,28 @@ createCustomOptimizer(const AC::PropsType &v) {
    * @param std::string key of the registerFactory
    * @param int int_key of the registerFactory
    */
  -class AppContextTest : public ::testing::TestWithParam<
  -        std::tuple<std::string, int> > {};
  +class AppContextTest
  +  : public ::testing::TestWithParam<std::tuple<std::string, int>> {};

   TEST_P(AppContextTest, RegisterCreateCustomOptimizer_p) {
  -    std::tuple<std::string, int> param = GetParam();
  -    std::string key = std::get<0>(param);
  -    int int_key = std::get<1>(param);
  -
  -    auto ac = nntrainer::AppContext();
  -    int num_id = ac.registerFactory(createCustomOptimizer, key, int_key);
  -    EXPECT_EQ(num_id, ((int_key == -1) ? (-1) * int_key : int_key));
  -    auto opt = ac.createObject<nntrainer::Optimizer>(
  -            ((key == "")? "identity_optimizer" : key), {});
  -    EXPECT_EQ(typeid(*opt).hash_code(), typeid(CustomOptimizer).hash_code());
  -    opt = ac.createObject<nntrainer::Optimizer>(num_id, {});
  -    EXPECT_EQ(typeid(*opt).hash_code(), typeid(CustomOptimizer).hash_code());
  +  std::tuple<std::string, int> param = GetParam();
  +  std::string key = std::get<0>(param);
  +  int int_key = std::get<1>(param);
  +
  +  auto ac = nntrainer::AppContext();
  +  int num_id = ac.registerFactory(createCustomOptimizer, key, int_key);
  +  EXPECT_EQ(num_id, ((int_key == -1) ? (-1) * int_key : int_key));
  +  auto opt = ac.createObject<nntrainer::Optimizer>(
  +    ((key == "") ? "identity_optimizer" : key), {});
  +  EXPECT_EQ(typeid(*opt).hash_code(), typeid(CustomOptimizer).hash_code());
  +  opt = ac.createObject<nntrainer::Optimizer>(num_id, {});
  +  EXPECT_EQ(typeid(*opt).hash_code(), typeid(CustomOptimizer).hash_code());
   }

   GTEST_PARAMETER_TEST(RegisterCreateCustomOptimizerTests, AppContextTest,
  -                    ::testing::Values(
  -                            std::make_tuple("", -1),
  -                            std::make_tuple("custom_key", -1),
  -                            std::make_tuple("custom_key", 5)
  -                    ));
  +                     ::testing::Values(std::make_tuple("", -1),
  +                                       std::make_tuple("custom_key", -1),
  +                                       std::make_tuple("custom_key", 5)));

   TEST(AppContextTest, RegisterFactoryWithClashingKey_n) {
     auto ac = nntrainer::AppContext();
::error clang-format has found style errors in C++ files.

please check this ! --> you can see this on checks detail

DonghakPark commented 3 months ago

@Boseong-Seo Again Thank you work !! but we also check commit message body exist. Although these tasks may be a bit cumbersome, we follow these rules for consistency and maintainability of the code. Can you please fill in the commit message and push again?

as you know it can be fix by git commit --amend command

Boseong-Seo commented 3 months ago

@Boseong-Seo Again Thank you work !! but we also check commit message body exist. Although these tasks may be a bit cumbersome, we follow these rules for consistency and maintainability of the code. Can you please fill in the commit message and push again?

as you know i can be fix by git commit --amend command

Oh I would try it! Thank you for the guidance

DonghakPark commented 3 months ago

@Boseong-Seo Again Thank you work !! but we also check commit message body exist. Although these tasks may be a bit cumbersome, we follow these rules for consistency and maintainability of the code. Can you please fill in the commit message and push again? as you know i can be fix by git commit --amend command

Oh I would try it! Thank you for the guidance

If you have any questions or concerns while working on a task, do not hesitate to reach out to us via @.

DonghakPark commented 3 months ago

@nnstreamer/nntrainer Please Take a look & Review