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

improve test failure msgs #35

Merged
merged 8 commits into from
Apr 16, 2024
Merged

improve test failure msgs #35

merged 8 commits into from
Apr 16, 2024

Conversation

aniketmaurya
Copy link
Collaborator

Before submitting
  • Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes # (issue).

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Merging #35 (8c27be5) into main (a75e342) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@        Coverage Diff         @@
##           main   #35   +/-   ##
==================================
  Coverage    81%   81%           
==================================
  Files         5     5           
  Lines       230   230           
==================================
  Hits        186   186           
  Misses       44    44           

@@ -17,14 +16,14 @@ def test_new_pipe(lit_server):
for _ in range(pool_size):
lit_server.new_pipe()

assert len(lit_server.pipe_pool) == 0
assert len(lit_server.new_pipe()) == 2
assert len(lit_server.pipe_pool) == 0, "pipe_pool was completely used and need to be empty"
Copy link
Contributor

Choose a reason for hiding this comment

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

can you describe it in non technical terms as well?

  • what in the code caused this issue?
  • etc...

Copy link
Collaborator Author

@aniketmaurya aniketmaurya Apr 12, 2024

Choose a reason for hiding this comment

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

@williamFalcon updated

@@ -101,7 +100,7 @@ def test_run():
)

time.sleep(5)
assert os.path.exists("client.py")
assert os.path.exists("client.py"), f"Expected client file to be created at {os.getcwd()} after starting the server"
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this is a great comment

output = subprocess.run("python client.py", shell=True, capture_output=True, text=True).stdout
assert '{"output":16.0}' in output
assert '{"output":16.0}' in output, "tests/simple_server.py didn't return expected output"
Copy link
Contributor

Choose a reason for hiding this comment

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

great comment

@williamFalcon williamFalcon merged commit 28aa1ae into main Apr 16, 2024
23 checks passed
@williamFalcon williamFalcon deleted the aniket/improve-test-msg branch April 16, 2024 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants