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

Http url on submit #527

Merged
merged 5 commits into from
Feb 19, 2019
Merged

Http url on submit #527

merged 5 commits into from
Feb 19, 2019

Conversation

dalazx
Copy link
Contributor

@dalazx dalazx commented Feb 19, 2019

This PR add outputting of a job's HTTP URL in neuro submit.

сс @astaff

@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #527 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
+ Coverage   92.05%   92.06%   +0.01%     
==========================================
  Files          40       40              
  Lines        2882     2886       +4     
  Branches      378      379       +1     
==========================================
+ Hits         2653     2657       +4     
  Misses        172      172              
  Partials       57       57
Impacted Files Coverage Δ
python/neuromation/cli/formatters/jobs.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17d7b2f...a4bbf5b. Read the comment docs.

@dalazx dalazx requested a review from asvetlov February 19, 2019 13:26
@atemate atemate self-requested a review February 19, 2019 13:29
Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

LGTM, except a small note:
e2e test is good but the change worth unit-test in tests/cli/test_formatters.py.
The reason is: uni-test is much easier to support than heavy-weight e2e one.
From my point of view, e2e is for testing integration with the server; but if the test can be implemented in unit style without mocking etc. -- let's do it.

I don't want to block you but very appreciate writing unit-test to reduce our tech debt and simplify future maintenance.

@dalazx
Copy link
Contributor Author

dalazx commented Feb 19, 2019

@asvetlov I'll address your comment in this PR. thanks!
I initially thought of having a unit test. I added a change and ran unit tests, but non failed. for some reason I thought that there were no unit tests for formatters.

@dalazx
Copy link
Contributor Author

dalazx commented Feb 19, 2019

@asvetlov addressed. sorry again.

@asvetlov
Copy link
Contributor

No problem.
Looks awesome, thank you!

@dalazx dalazx merged commit 1f136a8 into master Feb 19, 2019
@dalazx dalazx deleted the http_url_on_submit branch February 19, 2019 15:53
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.

5 participants