Skip to content

FC-0057: implement search test cases#76

Merged
Ali-Salman29 merged 1 commit intomasterfrom
alisalman/search-test-cases
Sep 6, 2024
Merged

FC-0057: implement search test cases#76
Ali-Salman29 merged 1 commit intomasterfrom
alisalman/search-test-cases

Conversation

@Ali-Salman29
Copy link
Copy Markdown
Contributor

@Ali-Salman29 Ali-Salman29 commented Sep 4, 2024

The PR includes the test cases for the search API along with some fixes in additional files.

  • Implemented test cases similar to those in the cs_comments_service/spec/api/search_spec.rb.
  • Added MongoDB indexes.
  • Fixed issues in the search API view.
  • By default, the test cases use fixtures to get dummy data. To test with the actual Elasticsearch, follow the guidelines mentioned in test_search.py.
  • Added relevant settings i.e. FORUM_DEFAULT_PAGE and FORUM_DEFAULT_PER_PAGE.

close #53

@Ali-Salman29 Ali-Salman29 force-pushed the alisalman/search-test-cases branch from fd7d919 to ad1c302 Compare September 4, 2024 09:07
@Ali-Salman29 Ali-Salman29 changed the title feat: add search test cases FC-0057: implement search test cases Sep 4, 2024
@Ali-Salman29
Copy link
Copy Markdown
Contributor Author

Ali-Salman29 commented Sep 4, 2024

@regisb To reproduce this error, first start the Elasticsearch container. Here, you can see that the port is different from the one we are using in the edx.

docker run --rm --name elasticsearch_container -p 5200:9200 -p 5300:9300 -e "discovery.type=single-node" -e "xpack.security.enabled=false" elasticsearch:7.17.13

To enable the use of elastic search instead of fixtures, update the value of FORUM_ENABLE_ELASTIC_SEARCH to True in the tests/conftest.py:33.

We are focused on fixing the issue mentioned in the forum/search/search_manager.py at line 80, and the relevant test case is check_correction in the file test_search.

To reproduce the error, comment out the temporary fix in the search_manager.py.

After doing so, the first test case will fail because Elasticsearch won't be able to find "pineapples" in the comments index.
Use the following test case to verify the error: check_correction("pinapples", "pineapples").

Use this command to run test cases for search only: pytest tests/test_views/test_search.py

@regisb
Copy link
Copy Markdown
Contributor

regisb commented Sep 4, 2024

I believe that the issue stems from the construction of the indices in v2. The only difference between v1 and v2 I managed to find was the content of the indices.

To identify the difference, I queried the content of the comments and comment_thread indices.

In v1:

$ curl http://elasticsearch:9200/comment_threads,comments/_search?pretty
{
  "took" : 1,
  "timed_out" : false,
  "_shards" : {
    "total" : 2,
    "successful" : 2,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 2,
      "relation" : "eq"
    },
    "max_score" : 1.0,
    "hits" : [
      {
        "_index" : "comment_threads_20240904150622420",
        "_type" : "_doc",
        "_id" : "66d8776f8c6533121e36067b",
        "_score" : 1.0,
        "_source" : {
          "abuse_flaggers" : [ ],
          "anonymous" : false,
          "anonymous_to_peers" : false,
          "at_position_list" : [ ],
          "author_id" : "cordell.mueller_5",
          "author_username" : "cordell.mueller_5",
          "body" : "a thread about green artichokes",
          "close_reason_code" : null,
          "closed" : false,
          "closed_by_id" : null,
          "comment_count" : 0,
          "commentable_id" : "test_commentable",
          "context" : "course",
          "course_id" : "test/course/id",
          "created_at" : "2024-09-04T15:06:23Z",
          "group_id" : null,
          "historical_abuse_flaggers" : [ ],
          "last_activity_at" : "2024-09-04T15:06:23Z",
          "pinned" : null,
          "retired_username" : null,
          "thread_type" : "discussion",
          "title" : "a thread about green artichokes",
          "updated_at" : "2024-09-04T15:06:23Z",
          "visible" : true,
          "votes" : {
            "up" : [ ],
            "down" : [ ],
            "up_count" : 0,
            "down_count" : 0,
            "count" : 0,
            "point" : 0
          }
        }
      },
      {
        "_index" : "comments_20240904150622420",
        "_type" : "_doc",
        "_id" : "66d8776f8c6533121e36067d",
        "_score" : 1.0,
        "_source" : {
          "abuse_flaggers" : [ ],
          "anonymous" : false,
          "anonymous_to_peers" : false,
          "at_position_list" : [ ],
          "author_id" : "cordell.mueller_5",
          "author_username" : "cordell.mueller_5",
          "body" : "a comment about greed pineapples",
          "child_count" : null,
          "comment_thread_id" : "66d8776f8c6533121e36067b",
          "commentable_id" : "test_commentable",
          "course_id" : "test/course/id",
          "created_at" : "2024-09-04T15:06:23Z",
          "depth" : 0,
          "endorsed" : false,
          "endorsement" : null,
          "historical_abuse_flaggers" : [ ],
          "parent_id" : null,
          "parent_ids" : [ ],
          "retired_username" : null,
          "sk" : "66d8776f8c6533121e36067d",
          "updated_at" : "2024-09-04T15:06:23Z",
          "visible" : true,
          "votes" : {
            "up" : [ ],
            "down" : [ ],
            "up_count" : 0,
            "down_count" : 0,
            "count" : 0,
            "point" : 0
          }
        }
      }
    ]
  }
}

In v2:

$ curl http://localhost:5200/comment_threads,comments/_search?pretty
{
  "took" : 2,
  "timed_out" : false,
  "_shards" : {
    "total" : 2,
    "successful" : 2,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 2,
      "relation" : "eq"
    },
    "max_score" : 1.0,
    "hits" : [
      {
        "_index" : "comment_threads_20240904095228",
        "_type" : "_doc",
        "_id" : "66d8742decb2a86851511014",
        "_score" : 1.0,
        "_source" : {
          "id" : "66d8742decb2a86851511014",
          "title" : "a thread about green artichokes",
          "body" : "text",
          "created_at" : "2024-09-04T09:52:29.049000",
          "updated_at" : "2024-09-04T09:52:29.049000",
          "last_activity_at" : "2024-09-04T09:52:29.049000",
          "comment_count" : 0,
          "votes_point" : 0,
          "context" : "course",
          "course_id" : "course_id",
          "commentable_id" : "test_commentable",
          "author_id" : "1",
          "group_id" : null,
          "thread_id" : "66d8742decb2a86851511014"
        }
      },
      {
        "_index" : "comments_20240904095228",
        "_type" : "_doc",
        "_id" : "66d8742decb2a86851511015",
        "_score" : 1.0,
        "_source" : {
          "body" : "a comment about greed pineapples",
          "course_id" : "course_id",
          "comment_thread_id" : "66d8742decb2a86851511014",
          "commentable_id" : null,
          "group_id" : null,
          "context" : "course",
          "created_at" : "2024-09-04T09:52:29.067000",
          "updated_at" : "2024-09-04T09:52:29.068000",
          "title" : null
        }
      }
    ]
  }
}

The index contents are very different. In particular, I think that the field mappings should be identical between comment_threads and comments. This is something that you could investigate (I think).

@Ali-Salman29
Copy link
Copy Markdown
Contributor Author

I implement it this way. There is a bug in the cs_comment service, which I have fixed in the V2 version. They store the exact MongoDB data to the index, but the mapping they created does not match the object. Also, in the search function, they are not utilizing any of the additional fields. We can still try this, but I don't think it will have a significant impact. However, I will give it a try.

https://github.com/openedx/cs_comments_service/blob/master/models/comment_thread.rb#L47

Comment thread forum/handlers.py Outdated
Comment thread forum/handlers.py Outdated
Comment thread forum/models/threads.py Outdated
Comment thread forum/views/search.py Outdated
Comment thread forum/views/search.py
Comment thread forum/views/search.py Outdated
Comment thread forum/search/search_manager.py Outdated
Comment thread test_settings.py Outdated
Comment thread tests/test_views/test_search.py Outdated
Comment thread tests/test_views/test_search.py Outdated
Comment thread forum/handlers.py Outdated
Comment thread forum/models/model_utils.py Outdated
Comment thread tests/test_views/test_search.py
Comment thread tests/test_views/test_search.py Outdated
@Ali-Salman29 Ali-Salman29 force-pushed the alisalman/search-test-cases branch from ce2d269 to 8ace927 Compare September 6, 2024 12:23
- Implemented test cases similar to those in the cs_comments_service/spec/api/search_spec.rb.
- Added MongoDB indexes.
- Fixed issues in the search API view.
- By default, the test cases use fixtures to get dummy data. To test with the actual Elasticsearch, follow the guidelines mentioned in test_search.py.
- Added relevant constants i.e. FORUM_DEFAULT_PAGE and FORUM_DEFAULT_PER_PAGE.

close #53
@Ali-Salman29 Ali-Salman29 force-pushed the alisalman/search-test-cases branch from 3219f32 to 20d28d6 Compare September 6, 2024 12:37
Comment thread forum/models/model_utils.py
Copy link
Copy Markdown
Contributor

@Faraz32123 Faraz32123 left a comment

Choose a reason for hiding this comment

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

LGTM!

@Ali-Salman29 Ali-Salman29 merged commit f281053 into master Sep 6, 2024
).limit(per_page)
threads = list(paginated_collection)
num_pages = max(1, (thread_count // per_page))
num_pages = max(1, math.ceil(thread_count / per_page))
Copy link
Copy Markdown
Contributor

@regisb regisb Sep 9, 2024

Choose a reason for hiding this comment

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

nit: The max(...) part is no longer necessary here. It used to be, because the // division could result in a zero result, but this is no longer the case here with ceil.

@regisb regisb deleted the alisalman/search-test-cases branch September 9, 2024 06:59
@regisb
Copy link
Copy Markdown
Contributor

regisb commented Sep 9, 2024

Good job figuring out this one :) Some parts of this PR were hardcore 🤘.

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.

Add test cases for search api

5 participants