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

GetTasks: allow for reverse param #603

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cosmastech
Copy link

@cosmastech cosmastech commented Jan 21, 2025

Pull Request

Related issue

Fixes #596

What does this PR do?

  • Allow passing "reverse" to the get tasks endpoint.

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@cosmastech
Copy link
Author

@Strift would the suggestion here be to write a completely new integration test inside of index_task_test.go that confirms the order matches? I don't think the other integration tests are actually confirming the output, but believe that maybe that would make sense?

@Strift
Copy link

Strift commented Jan 22, 2025

Hi @cosmastech, thanks for your PR.

Testing the output would actually be testing Meilisearch and not the integration IMO, but that can turn useful to spot upstream regressions. It might also be the simplest way to write the tests.

I have no experience in go, so please take my advice with a grain of salt. Here's what I would do, e.g. in JavaScript:

  1. Check the HTTP request contains the reverse param
  2. Check the HTTP response against a snapshot (i.e., ensure that from one version to the next, it is the same)

@Ja7ad might have better guidance.

Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.23%. Comparing base (55d3119) to head (25cd056).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #603      +/-   ##
==========================================
+ Coverage   86.22%   86.23%   +0.01%     
==========================================
  Files          14       14              
  Lines        2845     2848       +3     
==========================================
+ Hits         2453     2456       +3     
  Misses        282      282              
  Partials      110      110              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Ja7ad
Copy link
Collaborator

Ja7ad commented Jan 22, 2025

@cosmastech Please regenerate types using easyjson; please follow the contributing rules.

@cosmastech
Copy link
Author

@cosmastech Please regenerate types using easyjson; please follow the contributing rules.

Thanks for the guidance. I apparently read a different set of contributing guidelines 😆

@cosmastech cosmastech marked this pull request as ready for review January 22, 2025 23:34
@Ja7ad Ja7ad self-requested a review January 23, 2025 05:30
@Ja7ad
Copy link
Collaborator

Ja7ad commented Jan 23, 2025

@cosmastech Please fix test issue: https://github.com/meilisearch/meilisearch-go/actions/runs/12919048835/job/36039358559?pr=603#step:6:1039

You can run unit test on your local, need to run meilisearch dockerised.

@cosmastech cosmastech marked this pull request as draft January 23, 2025 10:09
@cosmastech
Copy link
Author

cosmastech commented Jan 26, 2025

@cosmastech Please fix test issue: https://github.com/meilisearch/meilisearch-go/actions/runs/12919048835/job/36039358559?pr=603#step:6:1039

You can run unit test on your local, need to run meilisearch dockerised.

I'm seeing this fail on local against main. It looks like how from is handled when fetching tasks is different. https://github.com/meilisearch/meilisearch/pull/5048/files#diff-954c9c29d6a80c3ffbfe336f52e5b706608187ff4afebd4171f39e6edfdf6d34R712-R717

To fix the broken test in this package, I can simply change the TestTasksWithParams parameter to From=200.

Was wondering if @Ja7ad you could confirm:

  1. Is this test broken on your local running again main? My docker container is running pkgVersion=1.12.7
  2. Should I fix this test so that it passes, even tho it seems like upstream behavior may have changed?

Thanks!

@cosmastech cosmastech marked this pull request as ready for review January 26, 2025 18:47
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.

[v1.12.0] Add reverse parameter for fetching tasks
3 participants