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

Wait for pending update and error naming #311

Merged
merged 3 commits into from
Apr 22, 2020

Conversation

bidoubiwa
Copy link
Contributor

In this PR

  • Some styling fixes

Wait For Pending Update

WaitForPendingUpdate function, based on this issue: meilisearch/integration-guides#1
With its associated tests.

New Error naming

  • The base MeiliSearch error is now called MeiliSearchApiError.
  • The timeout error in wait for Pending update is called MeiliSearchTimeOutError

@bidoubiwa
Copy link
Contributor Author

The tests are failing because they are not based on the 0.10. This will be changed in another PR

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

Great! Just some questions and remarks 🙂

src/utils.ts Show resolved Hide resolved
tests/wait_for_pending_update_tests.ts Outdated Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
Co-Authored-By: Clémentine Urquizar <clementine@meilisearch.com>
Copy link
Member

@eskombro eskombro left a comment

Choose a reason for hiding this comment

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

Sorry, just getting used to the syntax and everything. Good job!

But I am not sure the actual timeout is tested. Is it? As in, let's set a timeout of 5000ms, then not get a response on those 5000ms. I don't see that kind of tests in the file, only testing if expected behavior is met. Could be useful!

@bidoubiwa
Copy link
Contributor Author

bidoubiwa commented Apr 22, 2020

To be sure that MeiliSearch indexes in more than 5000 ms I would have to either store a huge json in the repo or download it on every CI.
I also would have to be sure that on every type of server the indexation will be more than 5sec. Because if the server is a little too powerful it could index in less than 5000ms.

I could also create a MOCK of meilisearch for the testing. Which is in fact the best-practice for wrapper testing. But since MeiliSearch is young we still need to test it in every way possible so the mocking has been postponed.

Whenever we arrive to the mocking, I think it is in fact a good idea to try out the timeout of 5s.

Unless of course, I'm missing on an easy way to do it and in which case I'm all ears.

@curquiza
Copy link
Member

curquiza commented Apr 22, 2020

@eskombro The actual timeout is tested in the last test Try to WaitForPendingStatus with small timeout and raise an error => the exception is thrown, that's what we expect from the timeout :)

The default value of 5seconds is not tested, I agree. But we can easily test the timeout by setting a weaker value as @bidoubiwa did in the last test. That's the most important :)

@eskombro
Copy link
Member

You are right @curquiza , the only thing is that we are not testing the default values or a real timeout situation, but a very edge case (0ms). But I agree with you, that should be enough!

@bidoubiwa some test libraries propose to override functions so you can, for example, fake a server behaviour. For instance, instead of pushing to a real MeiliSearch server, the function can be replaced by a sleep() plus a mock response. But I don't know how it works in JS testing libraries. An example of what I am talking about in Go could be https://github.com/bouk/monkey

I don't think this is required for now (I did approuve), but wanted to throw the idea!

@bidoubiwa
Copy link
Contributor Author

Then we agree to agree!
Its a great idea. Maybe create a subject about that in the integrations repo.

@bidoubiwa bidoubiwa merged commit 99c905f into remove_boilerplate Apr 22, 2020
@bidoubiwa bidoubiwa deleted the wait_for_pending_update branch April 22, 2020 12:23
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.

3 participants