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.

Sign up for GitHub

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

Jump to bottom

Add tests to load an actual spec #78

Merged
merged 3 commits into from
Oct 6, 2021
Merged

Add tests to load an actual spec #78

merged 3 commits into from
Oct 6, 2021

Conversation

yihuaf
Copy link
Contributor

@yihuaf yihuaf commented Oct 5, 2021 •
edited
Loading

  1. Without using an actual spec, we only tested spec generated by our code. If we mess up with the types, we will fail to notice it. The sample.json taken directly from `opencontainer/runtime-spec.
  2. Hook timeout should not be Duration. Duration implies a much richer structure. Here, the timeout means number of seconds, represented by an i64 integer. We can't parse that to a Duration without custom parser.

@yihuaf
Adds a test to run actual json spec.
f45fc12 
Without this, we will only test what's generated by our code, not what is expected.
@yihuaf
Revert "change hook timeout from Option<int64> to Option<time::Durati…
81d931e 
…on>"

This reverts commit 36cf113.

The hook timeout should be i64, not Duration. Duration implies a much
finer structure, but the field indicates specifically the number of
seconds before timeout. Duration is not the right choice here.
Reverting back to i64.
@yihuaf yihuaf mentioned this pull request Oct 5, 2021
Merged
@codecov-commenter
Copy link

Codecov Report

Merging #78 (81d931e) into main (19c26ca) will decrease coverage by 0.06%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
- Coverage   26.93%   26.87%   -0.07%     
==========================================
  Files          19       20       +1     
  Lines        1344     1362      +18     
  Branches      694      706      +12     
==========================================
+ Hits          362      366       +4     
+ Misses        374      367       -7     
- Partials      608      629      +21

@yihuaf
Copy link
Contributor Author

yihuaf commented Oct 6, 2021

@utam0k PTAL

utam0k
utam0k reviewed Oct 6, 2021
View reviewed changes
src/runtime/test.rs Outdated Show resolved Hide resolved
utam0k
utam0k approved these changes Oct 6, 2021
View reviewed changes
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

LGTM

@utam0k utam0k merged commit 3d5132a into youki-dev:main Oct 6, 2021
@yihuaf yihuaf deleted the yihuaf/hook branch October 6, 2021 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utam0k utam0k utam0k approved these changes

@saschagrunert saschagrunert saschagrunert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yihuaf @codecov-commenter @saschagrunert @utam0k