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

Increase the timeout to run tests/test_server.py::assert_trained_model test successfully on Windows #8781

Merged
merged 7 commits into from
Jun 1, 2021

Conversation

alwx
Copy link
Contributor

@alwx alwx commented May 31, 2021

Proposed changes:

The reason for this change: it sometimes takes longer to run the training process on Windows (everything is a bit slower on Windows GH runners), and that's why the test for tests/test_server.py::assert_trained_model is sometimes flakey. This change fixes it.

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@alwx alwx requested a review from ancalita May 31, 2021 13:13
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Great investigation work and resolution 💯

@@ -537,7 +538,7 @@ async def test_train_core_success_with(
assert os.path.exists(os.path.join(model_path, "fingerprint.json"))


async def test_train_with_retrieval_events_success(
Copy link
Member

Choose a reason for hiding this comment

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

Why did you edit the original test name? The name is now a duplicate of the test below that gets called in this test body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, this is a mistake. Fixed it.

@alwx alwx requested a review from ancalita June 1, 2021 09:37
@@ -0,0 +1 @@
Increase the timeout to run `tests/test_server.py::assert_trained_model` test successfully on Windows.
Copy link
Member

Choose a reason for hiding this comment

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

This could be nitpicking now - shouldn't the name of the test in the changelog also change? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is so stupid that the test name has somehow changed 🙈 Probably my mind was occupied with something else at this point.

Copy link
Contributor Author

@alwx alwx Jun 1, 2021

Choose a reason for hiding this comment

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

Fixed now. Sorry for spending your time

Copy link
Member

Choose a reason for hiding this comment

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

No worries :)

@alwx alwx requested a review from ancalita June 1, 2021 10:57
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

💯

@alwx alwx enabled auto-merge June 1, 2021 11:24
@alwx alwx merged commit 7d4164f into main Jun 1, 2021
@alwx alwx deleted the bug/8658 branch June 1, 2021 14:31
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.

Flakey CI test: Error empty file
2 participants