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

Livestream tests #135

Merged
merged 57 commits into from
Dec 23, 2021
Merged

Livestream tests #135

merged 57 commits into from
Dec 23, 2021

Conversation

Kishan-Dhakan
Copy link
Contributor

@Kishan-Dhakan Kishan-Dhakan commented Dec 15, 2021

  • revert actions back to master after actions PR approved
    Streaming upload coverage, cases:
  • Uploading youtube feed to allocation should work
  • upload from feed with delay flag must work
  • Upload from feed with a different chunksize must work
  • Uploading local webcam feed to allocation should work
  • Uploading local webcam feed to allocation with delay specified should work
  • Upload local webcam feed with a different chunksize must work
  • Uploading youtube feed with negative delay should fail
  • Upload from feed with a negative chunksize should fail
  • Uploading local webcam feed with negative delay should fail
  • Upload from feed with a negative chunksize should fail

FIXMEs found:

  • ceil(size/chunksize) is not equalling numblocks when chunksize is user-specified (it should be)
  • negative chunksize is not throwing any error but instead starts uploading (it shouldn't)

@stewartie4 stewartie4 linked an issue Dec 17, 2021 that may be closed by this pull request
@mohsenno1
Copy link
Contributor

Great work. Look ok to me.

@@ -110,7 +110,7 @@ jobs:
blobber_stake_image: latest

- name: "Run System tests"
uses: 0chain/actions/run-system-tests@master
uses: 0chain/actions/run-system-tests@streamupload
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to return it back to 'master'?

Copy link
Contributor

Choose a reason for hiding this comment

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

good point, is there an actions PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Just raised a PR on actions repo, once that is merged I'll make a commit to revert this so it gets tested again before merge👍

@stewartie4
Copy link
Contributor

@Kishan-Dhakan like you mentioned, these tests are hanging again - is there any logging we can add to debug why this is happening? We can't merge with this inconsistency as

  1. there may be a code issue
  2. we cant have everyone's CI hanging :(

@Kishan-Dhakan
Copy link
Contributor Author

@Kishan-Dhakan like you mentioned, these tests are hanging again - is there any logging we can add to debug why this is happening? We can't merge with this inconsistency as

1. there may be a code issue

2. we cant have everyone's CI hanging :(

Yes, definitely we can't merge this in it's current state :( I'm certain this is caused by child processes created by this tests (ffmpeg), which is a known issue in go. But I have set a pgid which should technically kill the child processes as well (confirmed on both win and ubuntu local machines thanks to @mohsenno1). I'm trying a few more things, will post an update by today

@stewartie4 stewartie4 merged commit 1a4edf9 into master Dec 23, 2021
@stewartie4 stewartie4 deleted the livestream-tests branch December 23, 2021 20:58
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.

livestream system tests
3 participants